From 7e105057a34c83cea542dacc55ff0528bce67afa Mon Sep 17 00:00:00 2001 From: Stefani Seibold Date: Fri, 15 Jan 2010 17:01:02 -0800 Subject: kfifo: fix kfifo_out_locked race bug Fix a wrong optimization in include/linux/kfifo.h which could cause a race in kfifo_out_locked. Signed-off-by: Stefani Seibold Reported-by: Johan Hovold Cc: Pete Zaitcev Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kfifo.h | 7 ------- 1 file changed, 7 deletions(-) (limited to 'include/linux/kfifo.h') diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index 7c6b32a1421c..c4ac88b3c302 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -228,13 +228,6 @@ static inline __must_check unsigned int kfifo_out_locked(struct kfifo *fifo, ret = kfifo_out(fifo, to, n); - /* - * optimization: if the FIFO is empty, set the indices to 0 - * so we don't wrap the next time - */ - if (kfifo_is_empty(fifo)) - kfifo_reset(fifo); - spin_unlock_irqrestore(lock, flags); return ret; -- cgit v1.2.3 From 8ecc2951534af10e04ddb5e5ff5c6d217b79f5c2 Mon Sep 17 00:00:00 2001 From: Andi Kleen Date: Fri, 15 Jan 2010 17:01:12 -0800 Subject: kfifo: use void * pointers for user buffers The pointers to user buffers are currently unsigned char *, which requires a lot of casting in the caller for any non-char typed buffers. Use void * instead. Signed-off-by: Andi Kleen Acked-by: Stefani Seibold Cc: Roland Dreier Cc: Dmitry Torokhov Cc: Andy Walls Cc: Vikram Dhillon Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kfifo.h | 10 +++++----- kernel/kfifo.c | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) (limited to 'include/linux/kfifo.h') diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index c4ac88b3c302..6fb495ea956a 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -104,15 +104,15 @@ union { \ #undef __kfifo_initializer -extern void kfifo_init(struct kfifo *fifo, unsigned char *buffer, +extern void kfifo_init(struct kfifo *fifo, void *buffer, unsigned int size); extern __must_check int kfifo_alloc(struct kfifo *fifo, unsigned int size, gfp_t gfp_mask); extern void kfifo_free(struct kfifo *fifo); extern unsigned int kfifo_in(struct kfifo *fifo, - const unsigned char *from, unsigned int len); + const void *from, unsigned int len); extern __must_check unsigned int kfifo_out(struct kfifo *fifo, - unsigned char *to, unsigned int len); + void *to, unsigned int len); /** * kfifo_reset - removes the entire FIFO contents @@ -194,7 +194,7 @@ static inline __must_check unsigned int kfifo_avail(struct kfifo *fifo) * bytes copied. */ static inline unsigned int kfifo_in_locked(struct kfifo *fifo, - const unsigned char *from, unsigned int n, spinlock_t *lock) + const void *from, unsigned int n, spinlock_t *lock) { unsigned long flags; unsigned int ret; @@ -219,7 +219,7 @@ static inline unsigned int kfifo_in_locked(struct kfifo *fifo, * @to buffer and returns the number of copied bytes. */ static inline __must_check unsigned int kfifo_out_locked(struct kfifo *fifo, - unsigned char *to, unsigned int n, spinlock_t *lock) + void *to, unsigned int n, spinlock_t *lock) { unsigned long flags; unsigned int ret; diff --git a/kernel/kfifo.c b/kernel/kfifo.c index e92d519f93b1..ab615e695052 100644 --- a/kernel/kfifo.c +++ b/kernel/kfifo.c @@ -28,7 +28,7 @@ #include #include -static void _kfifo_init(struct kfifo *fifo, unsigned char *buffer, +static void _kfifo_init(struct kfifo *fifo, void *buffer, unsigned int size) { fifo->buffer = buffer; @@ -44,7 +44,7 @@ static void _kfifo_init(struct kfifo *fifo, unsigned char *buffer, * @size: the size of the internal buffer, this have to be a power of 2. * */ -void kfifo_init(struct kfifo *fifo, unsigned char *buffer, unsigned int size) +void kfifo_init(struct kfifo *fifo, void *buffer, unsigned int size) { /* size must be a power of 2 */ BUG_ON(!is_power_of_2(size)); @@ -235,7 +235,7 @@ EXPORT_SYMBOL(__kfifo_in_n); * Note that with only one concurrent reader and one concurrent * writer, you don't need extra locking to use these functions. */ -unsigned int kfifo_in(struct kfifo *fifo, const unsigned char *from, +unsigned int kfifo_in(struct kfifo *fifo, const void *from, unsigned int len) { len = min(kfifo_avail(fifo), len); @@ -277,7 +277,7 @@ EXPORT_SYMBOL(__kfifo_out_n); * Note that with only one concurrent reader and one concurrent * writer, you don't need extra locking to use these functions. */ -unsigned int kfifo_out(struct kfifo *fifo, unsigned char *to, unsigned int len) +unsigned int kfifo_out(struct kfifo *fifo, void *to, unsigned int len) { len = min(kfifo_len(fifo), len); -- cgit v1.2.3 From 64ce1037c5434b1d036cd99ecaee6e00496bc2e9 Mon Sep 17 00:00:00 2001 From: Andi Kleen Date: Fri, 15 Jan 2010 17:01:15 -0800 Subject: kfifo: sanitize *_user error handling Right now for kfifo_*_user it's not easily possible to distingush between a user copy failing and the FIFO not containing enough data. The problem is that both conditions are multiplexed into the same return code. Avoid this by moving the "copy length" into a separate output parameter and only return 0/-EFAULT in the main return value. I didn't fully adapt the weird "record" variants, those seem to be unused anyways and were rather messy (should they be just removed?) I would appreciate some double checking if I did all the conversions correctly. Signed-off-by: Andi Kleen Cc: Stefani Seibold Cc: Roland Dreier Cc: Dmitry Torokhov Cc: Andy Walls Cc: Vikram Dhillon Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kfifo.h | 8 +++--- kernel/kfifo.c | 76 +++++++++++++++++++++++++++++++++------------------ 2 files changed, 53 insertions(+), 31 deletions(-) (limited to 'include/linux/kfifo.h') diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index 6fb495ea956a..86ad50a900c8 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -235,11 +235,11 @@ static inline __must_check unsigned int kfifo_out_locked(struct kfifo *fifo, extern void kfifo_skip(struct kfifo *fifo, unsigned int len); -extern __must_check unsigned int kfifo_from_user(struct kfifo *fifo, - const void __user *from, unsigned int n); +extern __must_check int kfifo_from_user(struct kfifo *fifo, + const void __user *from, unsigned int n, unsigned *lenout); -extern __must_check unsigned int kfifo_to_user(struct kfifo *fifo, - void __user *to, unsigned int n); +extern __must_check int kfifo_to_user(struct kfifo *fifo, + void __user *to, unsigned int n, unsigned *lenout); /* * __kfifo_add_out internal helper function for updating the out offset diff --git a/kernel/kfifo.c b/kernel/kfifo.c index ab615e695052..b50bb622e8b0 100644 --- a/kernel/kfifo.c +++ b/kernel/kfifo.c @@ -159,8 +159,9 @@ static inline void __kfifo_out_data(struct kfifo *fifo, memcpy(to + l, fifo->buffer, len - l); } -static inline unsigned int __kfifo_from_user_data(struct kfifo *fifo, - const void __user *from, unsigned int len, unsigned int off) +static inline int __kfifo_from_user_data(struct kfifo *fifo, + const void __user *from, unsigned int len, unsigned int off, + unsigned *lenout) { unsigned int l; int ret; @@ -177,16 +178,20 @@ static inline unsigned int __kfifo_from_user_data(struct kfifo *fifo, /* first put the data starting from fifo->in to buffer end */ l = min(len, fifo->size - off); ret = copy_from_user(fifo->buffer + off, from, l); - - if (unlikely(ret)) - return ret + len - l; + if (unlikely(ret)) { + *lenout = ret; + return -EFAULT; + } + *lenout = l; /* then put the rest (if any) at the beginning of the buffer */ - return copy_from_user(fifo->buffer, from + l, len - l); + ret = copy_from_user(fifo->buffer, from + l, len - l); + *lenout += ret ? ret : len - l; + return ret ? -EFAULT : 0; } -static inline unsigned int __kfifo_to_user_data(struct kfifo *fifo, - void __user *to, unsigned int len, unsigned int off) +static inline int __kfifo_to_user_data(struct kfifo *fifo, + void __user *to, unsigned int len, unsigned int off, unsigned *lenout) { unsigned int l; int ret; @@ -203,12 +208,21 @@ static inline unsigned int __kfifo_to_user_data(struct kfifo *fifo, /* first get the data from fifo->out until the end of the buffer */ l = min(len, fifo->size - off); ret = copy_to_user(to, fifo->buffer + off, l); - - if (unlikely(ret)) - return ret + len - l; + *lenout = l; + if (unlikely(ret)) { + *lenout -= ret; + return -EFAULT; + } /* then get the rest (if any) from the beginning of the buffer */ - return copy_to_user(to + l, fifo->buffer, len - l); + len -= l; + ret = copy_to_user(to + l, fifo->buffer, len); + if (unlikely(ret)) { + *lenout += len - ret; + return -EFAULT; + } + *lenout += len; + return 0; } unsigned int __kfifo_in_n(struct kfifo *fifo, @@ -299,10 +313,13 @@ EXPORT_SYMBOL(__kfifo_out_generic); unsigned int __kfifo_from_user_n(struct kfifo *fifo, const void __user *from, unsigned int len, unsigned int recsize) { + unsigned total; + if (kfifo_avail(fifo) < len + recsize) return len + 1; - return __kfifo_from_user_data(fifo, from, len, recsize); + __kfifo_from_user_data(fifo, from, len, recsize, &total); + return total; } EXPORT_SYMBOL(__kfifo_from_user_n); @@ -313,18 +330,21 @@ EXPORT_SYMBOL(__kfifo_from_user_n); * @len: the length of the data to be added. * * This function copies at most @len bytes from the @from into the - * FIFO depending and returns the number of copied bytes. + * FIFO depending and returns -EFAULT/0. * * Note that with only one concurrent reader and one concurrent * writer, you don't need extra locking to use these functions. */ -unsigned int kfifo_from_user(struct kfifo *fifo, - const void __user *from, unsigned int len) +int kfifo_from_user(struct kfifo *fifo, + const void __user *from, unsigned int len, unsigned *total) { + int ret; len = min(kfifo_avail(fifo), len); - len -= __kfifo_from_user_data(fifo, from, len, 0); + ret = __kfifo_from_user_data(fifo, from, len, 0, total); + if (ret) + return ret; __kfifo_add_in(fifo, len); - return len; + return 0; } EXPORT_SYMBOL(kfifo_from_user); @@ -339,17 +359,17 @@ unsigned int __kfifo_to_user_n(struct kfifo *fifo, void __user *to, unsigned int len, unsigned int reclen, unsigned int recsize) { - unsigned int ret; + unsigned int ret, total; if (kfifo_len(fifo) < reclen + recsize) return len; - ret = __kfifo_to_user_data(fifo, to, reclen, recsize); + ret = __kfifo_to_user_data(fifo, to, reclen, recsize, &total); if (likely(ret == 0)) __kfifo_add_out(fifo, reclen + recsize); - return ret; + return total; } EXPORT_SYMBOL(__kfifo_to_user_n); @@ -358,20 +378,22 @@ EXPORT_SYMBOL(__kfifo_to_user_n); * @fifo: the fifo to be used. * @to: where the data must be copied. * @len: the size of the destination buffer. + @ @lenout: pointer to output variable with copied data * * This function copies at most @len bytes from the FIFO into the - * @to buffer and returns the number of copied bytes. + * @to buffer and 0 or -EFAULT. * * Note that with only one concurrent reader and one concurrent * writer, you don't need extra locking to use these functions. */ -unsigned int kfifo_to_user(struct kfifo *fifo, - void __user *to, unsigned int len) +int kfifo_to_user(struct kfifo *fifo, + void __user *to, unsigned int len, unsigned *lenout) { + int ret; len = min(kfifo_len(fifo), len); - len -= __kfifo_to_user_data(fifo, to, len, 0); - __kfifo_add_out(fifo, len); - return len; + ret = __kfifo_to_user_data(fifo, to, len, 0, lenout); + __kfifo_add_out(fifo, *lenout); + return ret; } EXPORT_SYMBOL(kfifo_to_user); -- cgit v1.2.3 From a5b9e2c1063046421ce01dcf5ddd7ec12567f3e1 Mon Sep 17 00:00:00 2001 From: Andi Kleen Date: Fri, 15 Jan 2010 17:01:16 -0800 Subject: kfifo: add kfifo_out_peek In some upcoming code it's useful to peek into a FIFO without permanentely removing data. This patch implements a new kfifo_out_peek() to do this. Signed-off-by: Andi Kleen Acked-by: Stefani Seibold Cc: Roland Dreier Cc: Dmitry Torokhov Cc: Andy Walls Cc: Vikram Dhillon Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kfifo.h | 3 +++ kernel/kfifo.c | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) (limited to 'include/linux/kfifo.h') diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index 86ad50a900c8..7ad6d32dd673 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -113,6 +113,9 @@ extern unsigned int kfifo_in(struct kfifo *fifo, const void *from, unsigned int len); extern __must_check unsigned int kfifo_out(struct kfifo *fifo, void *to, unsigned int len); +extern __must_check unsigned int kfifo_out_peek(struct kfifo *fifo, + void *to, unsigned int len, unsigned offset); + /** * kfifo_reset - removes the entire FIFO contents diff --git a/kernel/kfifo.c b/kernel/kfifo.c index b50bb622e8b0..7384f120be87 100644 --- a/kernel/kfifo.c +++ b/kernel/kfifo.c @@ -302,6 +302,27 @@ unsigned int kfifo_out(struct kfifo *fifo, void *to, unsigned int len) } EXPORT_SYMBOL(kfifo_out); +/** + * kfifo_out_peek - copy some data from the FIFO, but do not remove it + * @fifo: the fifo to be used. + * @to: where the data must be copied. + * @len: the size of the destination buffer. + * @offset: offset into the fifo + * + * This function copies at most @len bytes at @offset from the FIFO + * into the @to buffer and returns the number of copied bytes. + * The data is not removed from the FIFO. + */ +unsigned int kfifo_out_peek(struct kfifo *fifo, void *to, unsigned int len, + unsigned offset) +{ + len = min(kfifo_len(fifo), len + offset); + + __kfifo_out_data(fifo, to, len, offset); + return len; +} +EXPORT_SYMBOL(kfifo_out_peek); + unsigned int __kfifo_out_generic(struct kfifo *fifo, void *to, unsigned int len, unsigned int recsize, unsigned int *total) -- cgit v1.2.3 From d994ffc247f7c4a48b848f10c4c01c9b06411ada Mon Sep 17 00:00:00 2001 From: Andi Kleen Date: Fri, 15 Jan 2010 17:01:17 -0800 Subject: kfifo: add kfifo_initialized Simple inline that checks if kfifo_init() has been executed on a fifo. This is useful for walking all per CPU fifos, when some of them might not have been brought up yet. Signed-off-by: Andi Kleen Acked-by: Stefani Seibold Cc: Roland Dreier Cc: Dmitry Torokhov Cc: Andy Walls Cc: Vikram Dhillon Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kfifo.h | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'include/linux/kfifo.h') diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index 7ad6d32dd673..c8618243ca5a 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -116,6 +116,16 @@ extern __must_check unsigned int kfifo_out(struct kfifo *fifo, extern __must_check unsigned int kfifo_out_peek(struct kfifo *fifo, void *to, unsigned int len, unsigned offset); +/** + * kfifo_initialized - Check if kfifo is initialized. + * @fifo: fifo to check + * Return %true if FIFO is initialized, otherwise %false. + * Assumes the fifo was 0 before. + */ +static inline bool kfifo_initialized(struct kfifo *fifo) +{ + return fifo->buffer != 0; +} /** * kfifo_reset - removes the entire FIFO contents -- cgit v1.2.3 From 5dab600e6a153ceb64832f608069e6c08185411a Mon Sep 17 00:00:00 2001 From: Andi Kleen Date: Fri, 15 Jan 2010 17:01:17 -0800 Subject: kfifo: document everywhere that size has to be power of two On my first try using them I missed that the fifos need to be power of two, resulting in a runtime bug. Document that requirement everywhere (and fix one grammar bug) Signed-off-by: Andi Kleen Acked-by: Stefani Seibold Cc: Roland Dreier Cc: Dmitry Torokhov Cc: Andy Walls Cc: Vikram Dhillon Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/kfifo.h | 4 ++-- kernel/kfifo.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'include/linux/kfifo.h') diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index c8618243ca5a..6f6c5f300af6 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -67,7 +67,7 @@ struct kfifo { /** * DECLARE_KFIFO - macro to declare a kfifo and the associated buffer * @name: name of the declared kfifo datatype - * @size: size of the fifo buffer + * @size: size of the fifo buffer. Must be a power of two. * * Note1: the macro can be used inside struct or union declaration * Note2: the macro creates two objects: @@ -91,7 +91,7 @@ union { \ /** * DEFINE_KFIFO - macro to define and initialize a kfifo * @name: name of the declared kfifo datatype - * @size: size of the fifo buffer + * @size: size of the fifo buffer. Must be a power of two. * * Note1: the macro can be used for global and local kfifo data type variables * Note2: the macro creates two objects: diff --git a/kernel/kfifo.c b/kernel/kfifo.c index 7384f120be87..32c5c15d750d 100644 --- a/kernel/kfifo.c +++ b/kernel/kfifo.c @@ -41,7 +41,7 @@ static void _kfifo_init(struct kfifo *fifo, void *buffer, * kfifo_init - initialize a FIFO using a preallocated buffer * @fifo: the fifo to assign the buffer * @buffer: the preallocated buffer to be used. - * @size: the size of the internal buffer, this have to be a power of 2. + * @size: the size of the internal buffer, this has to be a power of 2. * */ void kfifo_init(struct kfifo *fifo, void *buffer, unsigned int size) -- cgit v1.2.3