[PATCH RESEND 2/2] Input: uinput: Avoid returning 0 on read()

[PATCH RESEND 2/2] Input: uinput: Avoid returning 0 on read()

[David Herrmann]

Consider the output-queue to be almost full. A thread inside read() will
pass the wait_event_*() call and reach the while() loop. Now assume a new
message was added to the output-queue and the queue overruns, i.e.,
we now have udev->head == udev->tail.
The thread now passes the while() loop without fetching any message and
returns 0. However, at least for blocking FDs there is really no reason to
wake up user-space and for non-blocking FDs we should return -EAGAIN now.
Therefore, simply retry the read() if we didn't fetch any message.

We also check whether the user-supplied buffer is actually big enough and
return -EINVAL if it is not. This differs from current behavior which
caused 0 to be returned which actually does not make any sense. This may
break ABI since user-space programs might be used to get 0 if the buffer
is to small. However, 0 means the FD was closed so returning -EINVAL
*must* be handled similar in user-space, otherwise the programs are
broken.
Anyway, we need this check, otherwise we would have a never-returning
loop here because retval would always be 0.

Also note that an queue-overrun is not the only situation where this bug
occurs. We might also have a race between multiple threads here so we
definitely need to handle it this way.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
Acked-by: Aristeu Rozanski <aris@ruivo.org>
---
Hi Dmitry

Please note that this is based on my previous fix so you might get some
(trivial) conflicts if you didn't apply the previous one. They should be easy to
solve, though.
Also, the issue with returning -EINVAL if the buffer is too small and hence
breaking API can be resolved by moving the check down directly before running
"goto try_again;". However, I think returning -EINVAL is the better fix. Feel
free to change this, though.
To be honest, I also don't know whether read() actually returns 0 to user-space
if our handler returns 0 or if it changes this to anything else.

Regards
David

 drivers/input/misc/uinput.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 1526814..bf5cd7b 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -457,6 +457,10 @@ static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count,
 	struct uinput_device *udev = file->private_data;
 	int retval = 0;
 
+	if (count < input_event_size())
+		return -EINVAL;
+
+try_again:
 	if (udev->state != UIST_CREATED)
 		return -ENODEV;
 
@@ -490,6 +494,8 @@ static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count,
 
  out:
 	mutex_unlock(&udev->mutex);
+	if (!retval)
+		goto try_again;
 
 	return retval;
 }
-- 
1.7.9.4

[PATCH RESEND 1/2] Input: uinput: Fix race condition on read()

[David Herrmann]

Consider two threads calling read() on the same uinput-fd, both
non-blocking. Assume there is data-available so both will simultaneously
pass:
	udev->head == udev->tail
Then the first thread goes to sleep and the second one pops the message
from the queue. Now assume udev->head == udev->tail. If the first thread
wakes up it will call wait_event_*() and sleep in the waitq. This
effectively turns the non-blocking FD into a blocking one.

We fix this by never calling wait_event_*() for non-blocking FDs hence we
will never sleep in the waitq here.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
Acked-by: Aristeu Rozanski <aris@ruivo.org>
---
 drivers/input/misc/uinput.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 7360568..1526814 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -460,13 +460,15 @@ static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count,
 	if (udev->state != UIST_CREATED)
 		return -ENODEV;
 
-	if (udev->head == udev->tail && (file->f_flags & O_NONBLOCK))
-		return -EAGAIN;
-
-	retval = wait_event_interruptible(udev->waitq,
+	if (file->f_flags & O_NONBLOCK) {
+		if (udev->head == udev->tail)
+			return -EAGAIN;
+	} else {
+		retval = wait_event_interruptible(udev->waitq,
 			udev->head != udev->tail || udev->state != UIST_CREATED);
-	if (retval)
-		return retval;
+		if (retval)
+			return retval;
+	}
 
 	retval = mutex_lock_interruptible(&udev->mutex);
 	if (retval)
-- 
1.7.9.4

Re: [PATCH RESEND 2/2] Input: uinput: Avoid returning 0 on read()

[Dmitry Torokhov]

Hi David,

On Thu, Mar 29, 2012 at 02:14:04PM +0200, David Herrmann wrote:
> Consider the output-queue to be almost full. A thread inside read() will
> pass the wait_event_*() call and reach the while() loop. Now assume a new
> message was added to the output-queue and the queue overruns, i.e.,
> we now have udev->head == udev->tail.
> The thread now passes the while() loop without fetching any message and
> returns 0. However, at least for blocking FDs there is really no reason to
> wake up user-space and for non-blocking FDs we should return -EAGAIN now.
> Therefore, simply retry the read() if we didn't fetch any message.
> 
> We also check whether the user-supplied buffer is actually big enough and
> return -EINVAL if it is not. This differs from current behavior which
> caused 0 to be returned which actually does not make any sense. This may
> break ABI since user-space programs might be used to get 0 if the buffer
> is to small. However, 0 means the FD was closed so returning -EINVAL
> *must* be handled similar in user-space, otherwise the programs are
> broken.
> Anyway, we need this check, otherwise we would have a never-returning
> loop here because retval would always be 0.
> 
> Also note that an queue-overrun is not the only situation where this bug
> occurs. We might also have a race between multiple threads here so we
> definitely need to handle it this way.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> Acked-by: Aristeu Rozanski <aris@ruivo.org>
> ---
> Hi Dmitry
> 
> Please note that this is based on my previous fix so you might get some
> (trivial) conflicts if you didn't apply the previous one. They should be easy to
> solve, though.
> Also, the issue with returning -EINVAL if the buffer is too small and hence
> breaking API can be resolved by moving the check down directly before running
> "goto try_again;". However, I think returning -EINVAL is the better fix. Feel
> free to change this, though.

I agree that we should return -EINVAL when buffer is too small. I
however do not like the whole "try_again" business; I think it is
perfectly fine to return 0 for blocking reads, we just want to return
-EAGAIN for nonblocking.

I changed around your patches a bit and will post them shortly.

Aristeu, since the patches changed somewhat I dropped your Acked-by so
please Ack the patches you are comfortable with again.

Thanks.

-- 
Dmitry

Re: [PATCH RESEND 2/2] Input: uinput: Avoid returning 0 on read()

[David Herrmann]

Hi Dmitry

On Sat, Mar 31, 2012 at 8:00 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
<snip>
> I agree that we should return -EINVAL when buffer is too small. I
> however do not like the whole "try_again" business; I think it is
> perfectly fine to return 0 for blocking reads, we just want to return
> -EAGAIN for nonblocking.

The read() manpage says that return-code 0 means the fd got closed.
Does the VFS layer forward the return-code untouched to user-space or
why do you think returning 0 is fine? At least my uinput user-space
apps handle read()==0 as failure.

> I changed around your patches a bit and will post them shortly.

Apart from the ret==0 issue I have nothing to object. If you want to
apply them the way they're now, I am ok with it, too. Thanks for
cleaning them up.

> Aristeu, since the patches changed somewhat I dropped your Acked-by so
> please Ack the patches you are comfortable with again.
>
> Thanks.
>
> --
> Dmitry

Thanks
David

Re: [PATCH RESEND 2/2] Input: uinput: Avoid returning 0 on read()

[Dmitry Torokhov]

On Sat, Mar 31, 2012 at 10:39:49AM +0200, David Herrmann wrote:
> Hi Dmitry
> 
> On Sat, Mar 31, 2012 at 8:00 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi David,
> >
> <snip>
> > I agree that we should return -EINVAL when buffer is too small. I
> > however do not like the whole "try_again" business; I think it is
> > perfectly fine to return 0 for blocking reads, we just want to return
> > -EAGAIN for nonblocking.
> 
> The read() manpage says that return-code 0 means the fd got closed.
> Does the VFS layer forward the return-code untouched to user-space or
> why do you think returning 0 is fine? At least my uinput user-space
> apps handle read()==0 as failure.

Hmm, according to the spec:

http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html

it returns 0 to signal end of file, which does not make sense for
character devices, only regular files. I think I could also claim
that returning 0 when an event is "stolen" because "The behavior of
multiple concurrent reads on the same pipe, FIFO, or terminal device is
unspecified."

However I do not think that fixing it should be too hard, even taking
into account the special case of count == 0 outlined in the spec.
Below is the updated versions of the first 2 patches.

Thanks.

-- 
Dmitry


Input: uinput - return -EINVAL when read buffer size is too small

From: David Herrmann <dh.herrmann@googlemail.com>

Let's check whether the user-supplied buffer is actually big enough and
return -EINVAL if it is not. This differs from current behavior, which
caused 0 to be returned and actually does not make any sense, as
broken application will simply repeat the read getting into endless
loop.

Note that we treat 0 as a special case, according to the standard:

"Before any action described below is taken, and if nbyte is zero,
the read() function may detect and return errors as described below.
In the absence of errors, or if error detection is not performed,
the read() function shall return zero and have no other results."

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/misc/uinput.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)


diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 7360568..0386064 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -457,6 +457,9 @@ static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count,
 	struct uinput_device *udev = file->private_data;
 	int retval = 0;
 
+	if (count != 0 && count < input_event_size())
+		return -EINVAL;
+
 	if (udev->state != UIST_CREATED)
 		return -ENODEV;
 

Input: uinput - fix race that can block nonblocking read

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Consider two threads calling read() on the same uinput-fd, both
non-blocking. Assume there is data-available so both will simultaneously
pass:
	udev->head == udev->tail

Then the first thread goes to sleep and the second one pops the message
from the queue. Now assume udev->head == udev->tail. If the first thread
wakes up it will call wait_event_*() and sleep in the waitq. This
effectively turns the non-blocking FD into a blocking one.

We fix this by attempting to fetch events from the queue first and only
if we fail to retrieve any events we either return -EAGAIN (in case of
non-blocing read) or wait until there are more events.

This also fixes incorrect return code (we were returning 0 instead of
 -EAGAIN for non-blocking reads) when an event is "stolen" by another
thread. Blocking reads will now continue to wait instead of returning 0
in this scenario.

Count of 0 continues to be a special case, as per spec: we will check for
device existence and whether there are events in the queue, but no events
will be actually retrieved.

Reported-by: David Herrmann <dh.herrmann@googlemail.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/misc/uinput.c |   70 +++++++++++++++++++++++++------------------
 1 files changed, 41 insertions(+), 29 deletions(-)


diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 0386064..75e5502 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -452,45 +452,57 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, size_t
 	return retval;
 }
 
-static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
+static ssize_t uinput_events_to_user(struct uinput_device *udev,
+				     char __user *buffer, size_t count)
+{
+	size_t read = 0;
+	int error = 0;
+
+	while (udev->head != udev->tail &&
+               read + input_event_size() <= count) {
+		if (input_event_to_user(buffer + read,
+					&udev->buff[udev->tail])) {
+			error = -EFAULT;
+			break;
+		}
+		udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
+		read += input_event_size();
+	}
+
+	return read ?: error;
+}
+
+static ssize_t uinput_read(struct file *file, char __user *buffer,
+			   size_t count, loff_t *ppos)
 {
 	struct uinput_device *udev = file->private_data;
-	int retval = 0;
+	ssize_t retval;
 
 	if (count != 0 && count < input_event_size())
 		return -EINVAL;
 
-	if (udev->state != UIST_CREATED)
-		return -ENODEV;
+	do {
+		retval = mutex_lock_interruptible(&udev->mutex);
+		if (retval)
+			return retval;
 
-	if (udev->head == udev->tail && (file->f_flags & O_NONBLOCK))
-		return -EAGAIN;
+		if (udev->state != UIST_CREATED)
+			retval = -ENODEV;
+		else if (udev->head == udev->tail &&
+			 (file->f_flags & O_NONBLOCK))
+			retval = -EAGAIN;
+		else
+			retval = uinput_events_to_user(udev, buffer, count);
 
-	retval = wait_event_interruptible(udev->waitq,
-			udev->head != udev->tail || udev->state != UIST_CREATED);
-	if (retval)
-		return retval;
+		mutex_unlock(&udev->mutex);
 
-	retval = mutex_lock_interruptible(&udev->mutex);
-	if (retval)
-		return retval;
-
-	if (udev->state != UIST_CREATED) {
-		retval = -ENODEV;
-		goto out;
-	}
-
-	while (udev->head != udev->tail && retval + input_event_size() <= count) {
-		if (input_event_to_user(buffer + retval, &udev->buff[udev->tail])) {
-			retval = -EFAULT;
-			goto out;
-		}
-		udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
-		retval += input_event_size();
-	}
+		if (retval)
+			return retval;
 
- out:
-	mutex_unlock(&udev->mutex);
+		retval = wait_event_interruptible(udev->waitq,
+						  udev->head != udev->tail ||
+						  udev->state != UIST_CREATED);
+	} while (retval == 0);
 
 	return retval;
 }

Re: [PATCH RESEND 2/2] Input: uinput: Avoid returning 0 on read()

[David Herrmann]

Hi Dmitry

On Mon, Apr 2, 2012 at 9:48 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sat, Mar 31, 2012 at 10:39:49AM +0200, David Herrmann wrote:
>> Hi Dmitry
>>
>> On Sat, Mar 31, 2012 at 8:00 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi David,
>> >
>> <snip>
>> > I agree that we should return -EINVAL when buffer is too small. I
>> > however do not like the whole "try_again" business; I think it is
>> > perfectly fine to return 0 for blocking reads, we just want to return
>> > -EAGAIN for nonblocking.
>>
>> The read() manpage says that return-code 0 means the fd got closed.
>> Does the VFS layer forward the return-code untouched to user-space or
>> why do you think returning 0 is fine? At least my uinput user-space
>> apps handle read()==0 as failure.
>
> Hmm, according to the spec:
>
> http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
>
> it returns 0 to signal end of file, which does not make sense for
> character devices, only regular files. I think I could also claim
> that returning 0 when an event is "stolen" because "The behavior of
> multiple concurrent reads on the same pipe, FIFO, or terminal device is
> unspecified."

You're right. Then my applications didn't adhere to that correctly,
sorry. Anyway, I agree that fixing it to never return 0 does at least
prevent useless context-switches.

> However I do not think that fixing it should be too hard, even taking
> into account the special case of count == 0 outlined in the spec.
> Below is the updated versions of the first 2 patches.
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: uinput - return -EINVAL when read buffer size is too small
>
> From: David Herrmann <dh.herrmann@googlemail.com>
>
> Let's check whether the user-supplied buffer is actually big enough and
> return -EINVAL if it is not. This differs from current behavior, which
> caused 0 to be returned and actually does not make any sense, as
> broken application will simply repeat the read getting into endless
> loop.
>
> Note that we treat 0 as a special case, according to the standard:
>
> "Before any action described below is taken, and if nbyte is zero,
> the read() function may detect and return errors as described below.
> In the absence of errors, or if error detection is not performed,
> the read() function shall return zero and have no other results."
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Still looks fine. Thanks.

> ---
>
>  drivers/input/misc/uinput.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 7360568..0386064 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -457,6 +457,9 @@ static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count,
>        struct uinput_device *udev = file->private_data;
>        int retval = 0;
>
> +       if (count != 0 && count < input_event_size())
> +               return -EINVAL;
> +
>        if (udev->state != UIST_CREATED)
>                return -ENODEV;
>
>
> Input: uinput - fix race that can block nonblocking read
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Consider two threads calling read() on the same uinput-fd, both
> non-blocking. Assume there is data-available so both will simultaneously
> pass:
>        udev->head == udev->tail
>
> Then the first thread goes to sleep and the second one pops the message
> from the queue. Now assume udev->head == udev->tail. If the first thread
> wakes up it will call wait_event_*() and sleep in the waitq. This
> effectively turns the non-blocking FD into a blocking one.
>
> We fix this by attempting to fetch events from the queue first and only
> if we fail to retrieve any events we either return -EAGAIN (in case of
> non-blocing read) or wait until there are more events.
>
> This also fixes incorrect return code (we were returning 0 instead of
>  -EAGAIN for non-blocking reads) when an event is "stolen" by another
> thread. Blocking reads will now continue to wait instead of returning 0
> in this scenario.
>
> Count of 0 continues to be a special case, as per spec: we will check for
> device existence and whether there are events in the queue, but no events
> will be actually retrieved.
>
> Reported-by: David Herrmann <dh.herrmann@googlemail.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Ah, you replaced my goto try_again; with a do/while loop. Looks much
nicer now. Thanks!
I can't see any races anymore so I am fine with it.

Thank you!
David

> ---
>
>  drivers/input/misc/uinput.c |   70 +++++++++++++++++++++++++------------------
>  1 files changed, 41 insertions(+), 29 deletions(-)
>
>
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 0386064..75e5502 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -452,45 +452,57 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, size_t
>        return retval;
>  }
>
> -static ssize_t uinput_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
> +static ssize_t uinput_events_to_user(struct uinput_device *udev,
> +                                    char __user *buffer, size_t count)
> +{
> +       size_t read = 0;
> +       int error = 0;
> +
> +       while (udev->head != udev->tail &&
> +               read + input_event_size() <= count) {
> +               if (input_event_to_user(buffer + read,
> +                                       &udev->buff[udev->tail])) {
> +                       error = -EFAULT;
> +                       break;
> +               }
> +               udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
> +               read += input_event_size();
> +       }
> +
> +       return read ?: error;
> +}
> +
> +static ssize_t uinput_read(struct file *file, char __user *buffer,
> +                          size_t count, loff_t *ppos)
>  {
>        struct uinput_device *udev = file->private_data;
> -       int retval = 0;
> +       ssize_t retval;
>
>        if (count != 0 && count < input_event_size())
>                return -EINVAL;
>
> -       if (udev->state != UIST_CREATED)
> -               return -ENODEV;
> +       do {
> +               retval = mutex_lock_interruptible(&udev->mutex);
> +               if (retval)
> +                       return retval;
>
> -       if (udev->head == udev->tail && (file->f_flags & O_NONBLOCK))
> -               return -EAGAIN;
> +               if (udev->state != UIST_CREATED)
> +                       retval = -ENODEV;
> +               else if (udev->head == udev->tail &&
> +                        (file->f_flags & O_NONBLOCK))
> +                       retval = -EAGAIN;
> +               else
> +                       retval = uinput_events_to_user(udev, buffer, count);
>
> -       retval = wait_event_interruptible(udev->waitq,
> -                       udev->head != udev->tail || udev->state != UIST_CREATED);
> -       if (retval)
> -               return retval;
> +               mutex_unlock(&udev->mutex);
>
> -       retval = mutex_lock_interruptible(&udev->mutex);
> -       if (retval)
> -               return retval;
> -
> -       if (udev->state != UIST_CREATED) {
> -               retval = -ENODEV;
> -               goto out;
> -       }
> -
> -       while (udev->head != udev->tail && retval + input_event_size() <= count) {
> -               if (input_event_to_user(buffer + retval, &udev->buff[udev->tail])) {
> -                       retval = -EFAULT;
> -                       goto out;
> -               }
> -               udev->tail = (udev->tail + 1) % UINPUT_BUFFER_SIZE;
> -               retval += input_event_size();
> -       }
> +               if (retval)
> +                       return retval;
>
> - out:
> -       mutex_unlock(&udev->mutex);
> +               retval = wait_event_interruptible(udev->waitq,
> +                                                 udev->head != udev->tail ||
> +                                                 udev->state != UIST_CREATED);
> +       } while (retval == 0);
>
>        return retval;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html