staging: iio: iio_ring_rip_outer return immediately if rip_lots returns <= 0

staging: iio: iio_ring_rip_outer return immediately if rip_lots returns <= 0

[michael.hennerich]

The last two days I was casing a bug related sw ringbuffer usage.
Reading from an empty buffer caused my system to crash anytime later in
all kinds of fashions. The crash happened at a time where no iio related
function calls where going on.

It appears that on my nommu development system freeing memory twice can
lead to such bugs. And the fact that it does error right away made debugging
this issue a pain.

iio_ring_rip_outer() calls rb->access.rip_lots() which defaults in the
sw ring buffer case to iio_rip_sw_rb().

iio_rip_sw_rb() returns 0 and frees the local buffer:

	if (initial_write_p == initial_read_p) /* No new data available.*/
or
	if (unlikely(initial_read_p == NULL)) /* No data here as yet */

However iio_ring_rip_outer() only checks for  copied/retured  < 0.
It then carries on copies 0 bytes to user and frees the buffer again.

[PATCH] staging: iio: iio_ring_rip_outer return immediately if rip_lots returns <= 0

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>


Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/industrialio-ring.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/industrialio-ring.c b/drivers/staging/iio/industrialio-ring.c
index edcf6be..d393ace 100644
--- a/drivers/staging/iio/industrialio-ring.c
+++ b/drivers/staging/iio/industrialio-ring.c
@@ -105,7 +105,7 @@ static ssize_t iio_ring_rip_outer(struct file *filp, char __user *buf,
 		return -EINVAL;
 	copied = rb->access.rip_lots(rb, count, &data, &dead_offset);
 
-	if (copied < 0) {
+	if (copied <= 0) {
 		ret = copied;
 		goto error_ret;
 	}
-- 
1.6.0.2

[PATCH] staging: iio: iio_ring_rip_outer return immediately if rip_lots returns <= 0

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/industrialio-ring.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/iio/industrialio-ring.c b/drivers/staging/iio/industrialio-ring.c
index edcf6be..d393ace 100644
--- a/drivers/staging/iio/industrialio-ring.c
+++ b/drivers/staging/iio/industrialio-ring.c
@@ -105,7 +105,7 @@ static ssize_t iio_ring_rip_outer(struct file *filp, char __user *buf,
 		return -EINVAL;
 	copied = rb->access.rip_lots(rb, count, &data, &dead_offset);
 
-	if (copied < 0) {
+	if (copied <= 0) {
 		ret = copied;
 		goto error_ret;
 	}
-- 
1.6.0.2

Re: [PATCH] staging: iio: iio_ring_rip_outer return immediately if rip_lots returns <= 0

[Jonathan Cameron]

On 10/07/10 11:41, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> 
Diagnosis of the problem looks right.  Sorry about this one,
all my test cases only read from the buffer when they get an
event so I've never triggered this one. Guess I need another
test case!

I think freeing twice is going to cause trouble on any machine,
I've just never triggered the case.  I'll probably start
putting together a more comprehensive test program based
around the new generic_buffer program to hammer these
corner cases.

Thanks for your hard work.  You are nailing a
lot of bugs that have been there for quite a while.  

There is another issue somewhere in that code to do with
very small reads.  I triggered it due to a userspace bug
a while ago, but haven't had a chance to chase down the 
cause as yet.  Actually it seems fairly plausible that it
is another way of triggering the issue you have fixed here.

Great to get some eyes on that ring buffer code, it is a
fairly hideous mess!

> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/industrialio-ring.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/industrialio-ring.c b/drivers/staging/iio/industrialio-ring.c
> index edcf6be..d393ace 100644
> --- a/drivers/staging/iio/industrialio-ring.c
> +++ b/drivers/staging/iio/industrialio-ring.c
> @@ -105,7 +105,7 @@ static ssize_t iio_ring_rip_outer(struct file *filp, char __user *buf,
>  		return -EINVAL;
>  	copied = rb->access.rip_lots(rb, count, &data, &dead_offset);
>  
> -	if (copied < 0) {
> +	if (copied <= 0) {
>  		ret = copied;
>  		goto error_ret;
>  	}