[PATCH] Input: serio_raw - ensure we don't block in non-blocking read

[PATCH] Input: serio_raw - ensure we don't block in non-blocking read

[Dmitry Torokhov]

Avoid calling wait_event_interruptible() if client requested non-blocking
read, since it is not guaranteed that another thread will not consume
event after we checked if serio_raw->head != serio_raw->tail.

Also ensure we do not return 0 but keep waiting instead in blocking case,
when another thread steals "our" byte.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/serio/serio_raw.c |   43 ++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
index 4494233..094d6fb 100644
--- a/drivers/input/serio/serio_raw.c
+++ b/drivers/input/serio/serio_raw.c
@@ -165,31 +165,38 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
 	struct serio_raw *serio_raw = client->serio_raw;
 	char uninitialized_var(c);
 	ssize_t read = 0;
-	int retval;
+	int error = 0;
 
-	if (serio_raw->dead)
-		return -ENODEV;
+	do {
+		if (serio_raw->dead)
+			return -ENODEV;
 
-	if (serio_raw->head == serio_raw->tail && (file->f_flags & O_NONBLOCK))
-		return -EAGAIN;
+		if (serio_raw->head == serio_raw->tail &&
+		    (file->f_flags & O_NONBLOCK))
+			return -EAGAIN;
 
-	retval = wait_event_interruptible(serio_raw->wait,
-			serio_raw->head != serio_raw->tail || serio_raw->dead);
-	if (retval)
-		return retval;
+		if (count == 0)
+			break;
 
-	if (serio_raw->dead)
-		return -ENODEV;
+		while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
+			if (put_user(c, buffer++)) {
+				error = -EFAULT;
+				goto out;
+			}
+			read++;
+		}
 
-	while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
-		if (put_user(c, buffer++)) {
-			retval = -EFAULT;
+		if (read)
 			break;
-		}
-		read++;
-	}
 
-	return read ?: retval;
+		if (!(file->f_flags & O_NONBLOCK))
+			error = wait_event_interruptible(serio_raw->wait,
+					serio_raw->head != serio_raw->tail ||
+					serio_raw->dead);
+	} while (!error);
+
+out:
+	return read ?: error;
 }
 
 static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
-- 
1.7.7.6


-- 
Dmitry

Re: [PATCH] Input: serio_raw - ensure we don't block in non-blocking read

[Dmitry Torokhov]

On Tue, Apr 03, 2012 at 01:24:33PM -0700, Dmitry Torokhov wrote:
> Avoid calling wait_event_interruptible() if client requested non-blocking
> read, since it is not guaranteed that another thread will not consume
> event after we checked if serio_raw->head != serio_raw->tail.
> 
> Also ensure we do not return 0 but keep waiting instead in blocking case,
> when another thread steals "our" byte.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Anyone? It would be nice if someone could review my patches as well,
otherwise we may end up with breakages...

Thanks.

> ---
>  drivers/input/serio/serio_raw.c |   43 ++++++++++++++++++++++----------------
>  1 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
> index 4494233..094d6fb 100644
> --- a/drivers/input/serio/serio_raw.c
> +++ b/drivers/input/serio/serio_raw.c
> @@ -165,31 +165,38 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
>  	struct serio_raw *serio_raw = client->serio_raw;
>  	char uninitialized_var(c);
>  	ssize_t read = 0;
> -	int retval;
> +	int error = 0;
>  
> -	if (serio_raw->dead)
> -		return -ENODEV;
> +	do {
> +		if (serio_raw->dead)
> +			return -ENODEV;
>  
> -	if (serio_raw->head == serio_raw->tail && (file->f_flags & O_NONBLOCK))
> -		return -EAGAIN;
> +		if (serio_raw->head == serio_raw->tail &&
> +		    (file->f_flags & O_NONBLOCK))
> +			return -EAGAIN;
>  
> -	retval = wait_event_interruptible(serio_raw->wait,
> -			serio_raw->head != serio_raw->tail || serio_raw->dead);
> -	if (retval)
> -		return retval;
> +		if (count == 0)
> +			break;
>  
> -	if (serio_raw->dead)
> -		return -ENODEV;
> +		while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
> +			if (put_user(c, buffer++)) {
> +				error = -EFAULT;
> +				goto out;
> +			}
> +			read++;
> +		}
>  
> -	while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
> -		if (put_user(c, buffer++)) {
> -			retval = -EFAULT;
> +		if (read)
>  			break;
> -		}
> -		read++;
> -	}
>  
> -	return read ?: retval;
> +		if (!(file->f_flags & O_NONBLOCK))
> +			error = wait_event_interruptible(serio_raw->wait,
> +					serio_raw->head != serio_raw->tail ||
> +					serio_raw->dead);
> +	} while (!error);
> +
> +out:
> +	return read ?: error;
>  }
>  
>  static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
> -- 
> 1.7.7.6
> 

-- 
Dmitry

Re: [PATCH] Input: serio_raw - ensure we don't block in non-blocking read

[David Herrmann]

Hi Dmitry

On Wed, Apr 11, 2012 at 9:23 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Apr 03, 2012 at 01:24:33PM -0700, Dmitry Torokhov wrote:
>> Avoid calling wait_event_interruptible() if client requested non-blocking
>> read, since it is not guaranteed that another thread will not consume
>> event after we checked if serio_raw->head != serio_raw->tail.
>>
>> Also ensure we do not return 0 but keep waiting instead in blocking case,
>> when another thread steals "our" byte.
>>
>> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>
> Anyone? It would be nice if someone could review my patches as well,
> otherwise we may end up with breakages...

I cannot test the driver as I don't have the devices, but I reviewed
it and it looks fine.

> Thanks.

Regards
David

>> ---
>>  drivers/input/serio/serio_raw.c |   43 ++++++++++++++++++++++----------------
>>  1 files changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
>> index 4494233..094d6fb 100644
>> --- a/drivers/input/serio/serio_raw.c
>> +++ b/drivers/input/serio/serio_raw.c
>> @@ -165,31 +165,38 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
>>       struct serio_raw *serio_raw = client->serio_raw;
>>       char uninitialized_var(c);
>>       ssize_t read = 0;
>> -     int retval;
>> +     int error = 0;
>>
>> -     if (serio_raw->dead)
>> -             return -ENODEV;
>> +     do {
>> +             if (serio_raw->dead)
>> +                     return -ENODEV;
>>
>> -     if (serio_raw->head == serio_raw->tail && (file->f_flags & O_NONBLOCK))
>> -             return -EAGAIN;
>> +             if (serio_raw->head == serio_raw->tail &&
>> +                 (file->f_flags & O_NONBLOCK))
>> +                     return -EAGAIN;
>>
>> -     retval = wait_event_interruptible(serio_raw->wait,
>> -                     serio_raw->head != serio_raw->tail || serio_raw->dead);
>> -     if (retval)
>> -             return retval;
>> +             if (count == 0)
>> +                     break;
>>
>> -     if (serio_raw->dead)
>> -             return -ENODEV;
>> +             while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
>> +                     if (put_user(c, buffer++)) {
>> +                             error = -EFAULT;
>> +                             goto out;
>> +                     }
>> +                     read++;
>> +             }
>>
>> -     while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
>> -             if (put_user(c, buffer++)) {
>> -                     retval = -EFAULT;
>> +             if (read)
>>                       break;
>> -             }
>> -             read++;
>> -     }
>>
>> -     return read ?: retval;
>> +             if (!(file->f_flags & O_NONBLOCK))
>> +                     error = wait_event_interruptible(serio_raw->wait,
>> +                                     serio_raw->head != serio_raw->tail ||
>> +                                     serio_raw->dead);
>> +     } while (!error);
>> +
>> +out:
>> +     return read ?: error;
>>  }
>>
>>  static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
>> --
>> 1.7.7.6
>>
>
> --
> Dmitry

Re: [PATCH] Input: serio_raw - ensure we don't block in non-blocking read

[Che-liang Chiou]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3482 bytes --]

I didn't test the patch, but the logic looks fine to me.

On Wed, Apr 4, 2012 at 4:24 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Avoid calling wait_event_interruptible() if client requested non-blocking
> read, since it is not guaranteed that another thread will not consume
> event after we checked if serio_raw->head != serio_raw->tail.
>
> Also ensure we do not return 0 but keep waiting instead in blocking case,
> when another thread steals "our" byte.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>  drivers/input/serio/serio_raw.c |   43 ++++++++++++++++++++++----------------
>  1 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
> index 4494233..094d6fb 100644
> --- a/drivers/input/serio/serio_raw.c
> +++ b/drivers/input/serio/serio_raw.c
> @@ -165,31 +165,38 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
>        struct serio_raw *serio_raw = client->serio_raw;
>        char uninitialized_var(c);
>        ssize_t read = 0;
> -       int retval;
> +       int error = 0;
>
> -       if (serio_raw->dead)
> -               return -ENODEV;
> +       do {
> +               if (serio_raw->dead)
> +                       return -ENODEV;
>
> -       if (serio_raw->head == serio_raw->tail && (file->f_flags & O_NONBLOCK))
> -               return -EAGAIN;
> +               if (serio_raw->head == serio_raw->tail &&
> +                   (file->f_flags & O_NONBLOCK))
> +                       return -EAGAIN;
>
> -       retval = wait_event_interruptible(serio_raw->wait,
> -                       serio_raw->head != serio_raw->tail || serio_raw->dead);
> -       if (retval)
> -               return retval;
> +               if (count == 0)
> +                       break;
>
> -       if (serio_raw->dead)
> -               return -ENODEV;
> +               while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
> +                       if (put_user(c, buffer++)) {
> +                               error = -EFAULT;
> +                               goto out;
> +                       }
> +                       read++;
> +               }
>
> -       while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
> -               if (put_user(c, buffer++)) {
> -                       retval = -EFAULT;
> +               if (read)
>                        break;
> -               }
> -               read++;
> -       }
>
> -       return read ?: retval;
> +               if (!(file->f_flags & O_NONBLOCK))
> +                       error = wait_event_interruptible(serio_raw->wait,
> +                                       serio_raw->head != serio_raw->tail ||
> +                                       serio_raw->dead);
> +       } while (!error);
> +
> +out:
> +       return read ?: error;
>  }
>
>  static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
> --
> 1.7.7.6
>
>
> --
> Dmitry
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] Input: serio_raw - ensure we don't block in non-blocking read

[Che-liang Chiou]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3700 bytes --]

Reviewed-by: Che-Liang Chiou <clchiou@chromium.org>

On Tue, Apr 17, 2012 at 4:56 PM, Che-liang Chiou <clchiou@chromium.org> wrote:
> I didn't test the patch, but the logic looks fine to me.
>
> On Wed, Apr 4, 2012 at 4:24 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> Avoid calling wait_event_interruptible() if client requested non-blocking
>> read, since it is not guaranteed that another thread will not consume
>> event after we checked if serio_raw->head != serio_raw->tail.
>>
>> Also ensure we do not return 0 but keep waiting instead in blocking case,
>> when another thread steals "our" byte.
>>
>> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>> ---
>>  drivers/input/serio/serio_raw.c |   43 ++++++++++++++++++++++----------------
>>  1 files changed, 25 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
>> index 4494233..094d6fb 100644
>> --- a/drivers/input/serio/serio_raw.c
>> +++ b/drivers/input/serio/serio_raw.c
>> @@ -165,31 +165,38 @@ static ssize_t serio_raw_read(struct file *file, char __user *buffer,
>>        struct serio_raw *serio_raw = client->serio_raw;
>>        char uninitialized_var(c);
>>        ssize_t read = 0;
>> -       int retval;
>> +       int error = 0;
>>
>> -       if (serio_raw->dead)
>> -               return -ENODEV;
>> +       do {
>> +               if (serio_raw->dead)
>> +                       return -ENODEV;
>>
>> -       if (serio_raw->head == serio_raw->tail && (file->f_flags & O_NONBLOCK))
>> -               return -EAGAIN;
>> +               if (serio_raw->head == serio_raw->tail &&
>> +                   (file->f_flags & O_NONBLOCK))
>> +                       return -EAGAIN;
>>
>> -       retval = wait_event_interruptible(serio_raw->wait,
>> -                       serio_raw->head != serio_raw->tail || serio_raw->dead);
>> -       if (retval)
>> -               return retval;
>> +               if (count == 0)
>> +                       break;
>>
>> -       if (serio_raw->dead)
>> -               return -ENODEV;
>> +               while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
>> +                       if (put_user(c, buffer++)) {
>> +                               error = -EFAULT;
>> +                               goto out;
>> +                       }
>> +                       read++;
>> +               }
>>
>> -       while (read < count && serio_raw_fetch_byte(serio_raw, &c)) {
>> -               if (put_user(c, buffer++)) {
>> -                       retval = -EFAULT;
>> +               if (read)
>>                        break;
>> -               }
>> -               read++;
>> -       }
>>
>> -       return read ?: retval;
>> +               if (!(file->f_flags & O_NONBLOCK))
>> +                       error = wait_event_interruptible(serio_raw->wait,
>> +                                       serio_raw->head != serio_raw->tail ||
>> +                                       serio_raw->dead);
>> +       } while (!error);
>> +
>> +out:
>> +       return read ?: error;
>>  }
>>
>>  static ssize_t serio_raw_write(struct file *file, const char __user *buffer,
>> --
>> 1.7.7.6
>>
>>
>> --
>> Dmitry
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥