[patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[Davide Libenzi]

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 <davidel@xmailserver.org>


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

Re: [patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[Eric Dumazet]

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 <davidel@xmailserver.org>
> 
> 
> - 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.

Re: [patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[Eric Dumazet]

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

Re: [patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[Davide Libenzi]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1797 bytes --]

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 <davidel@xmailserver.org>
> > 
> > 
> > - 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

Re: [patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[THIELL Stephane]

> 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

Re: [patch/rfc] Make poll/select report error (POLLNVAL and EBADF) for unsupported files

[Davide Libenzi]

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