Currently poll and select consider a non poll-supported file as one with
full event mask set, instead of reporting proper error to the caller.
This behavior can fool the caller of proper functionality being returned,
while instead no valid event was processed/read from the device.
This came out linked to this bug report:
http://bugzilla.kernel.org/show_bug.cgi?id=15272
IMHO, it'd be more adequate to report proper error code, for files that do
not support f_op->poll(), but then I am also not sure how much breakage
can this bring to existing (already broken "in just the right way")
applications.
Untested, discussion-only, patch.
Signed-off-by: Davide Libenzi <[email protected]>
- Davide
---
fs/select.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
Index: linux-2.6.mod/fs/select.c
===================================================================
--- linux-2.6.mod.orig/fs/select.c 2010-02-14 14:06:12.000000000 -0800
+++ linux-2.6.mod/fs/select.c 2010-02-14 14:16:09.000000000 -0800
@@ -447,12 +447,13 @@ int do_select(int n, fd_set_bits *fds, s
continue;
file = fget_light(i, &fput_needed);
if (file) {
- f_op = file->f_op;
- mask = DEFAULT_POLLMASK;
- if (f_op && f_op->poll) {
- wait_key_set(wait, in, out, bit);
- mask = (*f_op->poll)(file, wait);
+ if (unlikely((f_op = file->f_op) == NULL ||
+ f_op->poll == NULL)) {
+ retval = -EBADF;
+ goto error_exit;
}
+ wait_key_set(wait, in, out, bit);
+ mask = (*f_op->poll)(file, wait);
fput_light(file, fput_needed);
if ((mask & POLLIN_SET) && (in & bit)) {
res_in |= bit;
@@ -501,7 +502,7 @@ int do_select(int n, fd_set_bits *fds, s
to, slack))
timed_out = 1;
}
-
+error_exit:
poll_freewait(&table);
return retval;
@@ -720,15 +721,14 @@ static inline unsigned int do_pollfd(str
file = fget_light(fd, &fput_needed);
mask = POLLNVAL;
if (file != NULL) {
- mask = DEFAULT_POLLMASK;
if (file->f_op && file->f_op->poll) {
if (pwait)
pwait->key = pollfd->events |
POLLERR | POLLHUP;
mask = file->f_op->poll(file, pwait);
+ /* Mask out unneeded events. */
+ mask &= pollfd->events | POLLERR | POLLHUP;
}
- /* Mask out unneeded events. */
- mask &= pollfd->events | POLLERR | POLLHUP;
fput_light(file, fput_needed);
}
}
Le dimanche 14 février 2010 à 14:27 -0800, Davide Libenzi a écrit :
> Currently poll and select consider a non poll-supported file as one with
> full event mask set, instead of reporting proper error to the caller.
> This behavior can fool the caller of proper functionality being returned,
> while instead no valid event was processed/read from the device.
> This came out linked to this bug report:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=15272
>
> IMHO, it'd be more adequate to report proper error code, for files that do
> not support f_op->poll(), but then I am also not sure how much breakage
> can this bring to existing (already broken "in just the right way")
> applications.
> Untested, discussion-only, patch.
>
>
> Signed-off-by: Davide Libenzi <[email protected]>
>
>
> - Davide
Hmm, according to POSIX :
The poll() function shall support regular files, terminal and
pseudo-terminal devices, FIFOs, pipes, sockets ...
Regular files shall always poll TRUE for reading and writing.
So unless I missed something, this patch could break some conformant
applications.
In particular, if an application is polling() on stdin (usually a tty),
and other 'files', what's happening if we do :
cat replay_file | application
Either it wont read stdin, or application exits without reading its
input.
Le lundi 15 février 2010 à 18:42 +0100, Eric Dumazet a écrit :
> Hmm, according to POSIX :
>
> The poll() function shall support regular files, terminal and
> pseudo-terminal devices, FIFOs, pipes, sockets ...
>
> Regular files shall always poll TRUE for reading and writing.
>
>
> So unless I missed something, this patch could break some conformant
> applications.
>
> In particular, if an application is polling() on stdin (usually a tty),
> and other 'files', what's happening if we do :
>
> cat replay_file | application
Doh..
I meant : application < replay_file
>
> Either it wont read stdin, or application exits without reading its
> input.
>
On Mon, 15 Feb 2010, Eric Dumazet wrote:
> Le dimanche 14 février 2010 à 14:27 -0800, Davide Libenzi a écrit :
> > Currently poll and select consider a non poll-supported file as one with
> > full event mask set, instead of reporting proper error to the caller.
> > This behavior can fool the caller of proper functionality being returned,
> > while instead no valid event was processed/read from the device.
> > This came out linked to this bug report:
> >
> > http://bugzilla.kernel.org/show_bug.cgi?id=15272
> >
> > IMHO, it'd be more adequate to report proper error code, for files that do
> > not support f_op->poll(), but then I am also not sure how much breakage
> > can this bring to existing (already broken "in just the right way")
> > applications.
> > Untested, discussion-only, patch.
> >
> >
> > Signed-off-by: Davide Libenzi <[email protected]>
> >
> >
> > - Davide
>
> Hmm, according to POSIX :
>
> The poll() function shall support regular files, terminal and
> pseudo-terminal devices, FIFOs, pipes, sockets ...
>
> Regular files shall always poll TRUE for reading and writing.
>
>
> So unless I missed something, this patch could break some conformant
> applications.
If that's POSIX, than the patch is no good. I missed the part on the
regular files being always ready, which explaqins why the code is as it is
right now.
> In particular, if an application is polling() on stdin (usually a tty),
> and other 'files', what's happening if we do :
>
> cat replay_file | application
>
> Either it wont read stdin, or application exits without reading its
> input.
That case would be fine, since that's a pipe.
This will behave differently:
$ application < replay_file
Anyway, since POSIX states the above, the patch cannot apply.
- Davide
> On Mon, 15 Feb 2010, Eric Dumazet wrote:
>
>
>> Hmm, according to POSIX :
>>
>> The poll() function shall support regular files, terminal and
>> pseudo-terminal devices, FIFOs, pipes, sockets ...
>>
>> Regular files shall always poll TRUE for reading and writing.
>>
>>
As POSIX says poll(2) have to support regular files (and it seems all
possible user file descriptors), then wouldn't it be better/more
coherent to have epoll(7) behave the same way (ie. support regular files
instead of epoll_ctl(2) returning EPERM), in order to allow generic code
handling both very common situations like:
$ cat replay_file | application
and
$ application < replay_file
...where for instance the application doesn't know the origine of its fd 0 (pipe, file, or something else).
Regards,
Stephane Thiell
On Wed, 17 Feb 2010, THIELL Stephane wrote:
>
> > On Mon, 15 Feb 2010, Eric Dumazet wrote:
> >
> >
> > > Hmm, according to POSIX :
> > >
> > > The poll() function shall support regular files, terminal and
> > > pseudo-terminal devices, FIFOs, pipes, sockets ...
> > >
> > > Regular files shall always poll TRUE for reading and writing.
> > >
> > >
> As POSIX says poll(2) have to support regular files (and it seems all possible
> user file descriptors), then wouldn't it be better/more coherent to have
> epoll(7) behave the same way (ie. support regular files instead of
> epoll_ctl(2) returning EPERM), in order to allow generic code handling both
> very common situations like:
>
> $ cat replay_file | application
> and
> $ application < replay_file
>
> ...where for instance the application doesn't know the origine of its fd 0
> (pipe, file, or something else).
Most definitely no. POSIX behavior is just stupid, and should have been
implemented by not having to report things/events that are not true.
Now, that's POSIX, and we cannot break its APIs.
New ones though, do not need to be necessarily stupid.
Besides, epoll needs an f_op->poll to work, and this would require all FS
files to implement a bogus f_op->poll which simply returns the full event
mask.
Which is just plain silly.
For things like epoll ET, this will simply not work, and even for LT,
you'd get every time out of epoll_wait() because you have something there
that always reports every event you ask, even though it is not true.
- Davide