Re: [GIT PULL] HID fixes

[Linus Torvalds]

On Thu, Jan 9, 2020 at 12:38 PM Jiri Kosina <jikos@xxxxxxxxxx> wrote:
>
> From: Jiri Kosina <jkosina@xxxxxxx>
> Subject: [PATCH] HID: hidraw, uhid: Always report EPOLLOUT

This looks fine, and certainly fixes the immediate problem.

However, this part could still be maybe be improved on:

>  static __poll_t hidraw_poll(struct file *file, poll_table *wait)
>  {
>         struct hidraw_list *list = file->private_data;
> +       __poll_t mask = EPOLLOUT | EPOLLWRNORM; /* hidraw is always writable */
>
>         poll_wait(file, &list->hidraw->wait, wait);
>         if (list->head != list->tail)
> -               return EPOLLIN | EPOLLRDNORM;
> +               mask |= EPOLLIN | EPOLLRDNORM;
>         if (!list->hidraw->exist)
>                 return EPOLLERR | EPOLLHUP;
> -       return EPOLLOUT | EPOLLWRNORM;
> +       return mask;
>  }

Notice the error condition.

I didn't actually _check_ what happens for errors, but *usually* error
conditions mean that

 (a) if there is data to be read, that is done before the error anyway

 (b) that even if there isn't data, the thing is "readable" and
"writable" in the sense that the op will return immediately (with an
error)

 (c) and most importantly - people may not be actually waiting for
EPOLLERR / EPOLLHUP at all.

Now, for "select()" we actually always turn EPOLLERR into "it's
readable _and_ writable". So if a user program uses "select()" to poll
for something, it will see it as being ready for IO in the error
condition.

But if somebody actually uses poll(), and asks for "let me know when
it is either readable or writable", look at what  happens above if
there is a pending error.

You'll basically tell the user that it's neither readable nor
writable, even if you otherwise would have set "EPOLLIN" in the mask.

So at a minimum, please do

          if (!list->hidraw->exist)
                  mask |= EPOLLERR | EPOLLHUP;

but quite possibly it may be a good idea to consider error conditions
to also mean EPOLLIN | EPOLLOUT, since _presumably_ that error will
also cause read/write to return immediately.

But again, I didn't actually verify that last part.

The exact semantics of EPOLLERR and EPOLLHUP really aren't 100% clear.
Should they set EPOLLIN and EPOLLOUT too - there may not be any data,
but at least a read or write won't block? Maybe? So that last
suggestion is questionable, but doing the "mask |=" part really is
unquestionably a better idea, since at least you don't want an error
to _hide_ existing data that is readable.

Now, good source code presumably notices EPOLLERR and handles it. So
it _shouldn't_ matter what the kernel does if an error occurs. I
haven't checked what people _actually_ do, tnough. I worry sometimes
that user space just looks at EPOLLIN sees it not being set, and gets
stuck in a busy loop polling in case of errors.

And since errors presumably don't actually happen much in real life,
it might not get a lot of coverage.

                 Linus