Hi Dmitry On Mon, Apr 2, 2012 at 9:48 AM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx> 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@xxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
