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

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 <redacted>
Acked-by: Aristeu Rozanski <redacted>
---
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

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 <redacted>
Acked-by: Aristeu Rozanski <redacted>
---
 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

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 <redacted>
Acked-by: Aristeu Rozanski <redacted>
---
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

Hi Dmitry

On Sat, Mar 31, 2012 at 8:00 AM, Dmitry Torokhov
[off-list ref] 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

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
[off-list ref] wrote:

quoted

Hi David,

<snip>

quoted

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 <redacted>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

 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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

 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;
 }

Hi Dmitry

On Mon, Apr 2, 2012 at 9:48 AM, Dmitry Torokhov
[off-list ref] wrote:

On Sat, Mar 31, 2012 at 10:39:49AM +0200, David Herrmann wrote:

quoted

Hi Dmitry

On Sat, Mar 31, 2012 at 8:00 AM, Dmitry Torokhov
[off-list ref] wrote:

quoted

Hi David,

<snip>

quoted

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 <redacted>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>

Still looks fine. Thanks.

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

--
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