[PATCH] Fix poll() nfds check.

[PATCH] Fix poll() nfds check.

[Vadim Lobanov]

Hi,

This is a trivial patch to fix the nfds check in the poll system call
implementation. Namely, OPEN_MAX no longer does anything important in
the kernel, and checking that nfds is greater than max_fdset AND greater
than OPEN_MAX therefore just seems wrong.

This brings up a slightly-tangential question: Why do the nfds checks
exist in select()/poll()? They're not strictly necessary, since bad
input will be caught later when we validate all the fds, one by one.
Furthermore, these checks optimize the handling of error cases (which
should be uncommon) while pessimizing correct usage of the syscalls
(which should be more common).

Signed-off-by: Vadim Lobanov <vlobanov@speakeasy.net>

diff -Npru linux-2.6.17-git25/fs/select.c linux-new/fs/select.c
--- linux-2.6.17-git25/fs/select.c	2006-07-05 19:06:56.000000000 -0700
+++ linux-new/fs/select.c	2006-07-05 19:10:51.000000000 -0700
@@ -671,7 +671,7 @@ int do_sys_poll(struct pollfd __user *uf
 	fdt = files_fdtable(current->files);
 	max_fdset = fdt->max_fdset;
 	rcu_read_unlock();
-	if (nfds > max_fdset && nfds > OPEN_MAX)
+	if (nfds > max_fdset)
 		return -EINVAL;

 	poll_initwait(&table);

Re: [PATCH] Fix poll() nfds check.

[Andrew Morton]

On Wed, 5 Jul 2006 20:00:34 -0700 (PDT)
Vadim Lobanov <vlobanov@speakeasy.net> wrote:

> Hi,
> 
> This is a trivial patch to fix the nfds check in the poll system call
> implementation. Namely, OPEN_MAX no longer does anything important in
> the kernel, and checking that nfds is greater than max_fdset AND greater
> than OPEN_MAX therefore just seems wrong.
> 
> This brings up a slightly-tangential question: Why do the nfds checks
> exist in select()/poll()? They're not strictly necessary, since bad
> input will be caught later when we validate all the fds, one by one.
> Furthermore, these checks optimize the handling of error cases (which
> should be uncommon) while pessimizing correct usage of the syscalls
> (which should be more common).
> 
> Signed-off-by: Vadim Lobanov <vlobanov@speakeasy.net>
> 
> diff -Npru linux-2.6.17-git25/fs/select.c linux-new/fs/select.c
> --- linux-2.6.17-git25/fs/select.c	2006-07-05 19:06:56.000000000 -0700
> +++ linux-new/fs/select.c	2006-07-05 19:10:51.000000000 -0700
> @@ -671,7 +671,7 @@ int do_sys_poll(struct pollfd __user *uf
>  	fdt = files_fdtable(current->files);
>  	max_fdset = fdt->max_fdset;
>  	rcu_read_unlock();
> -	if (nfds > max_fdset && nfds > OPEN_MAX)
> +	if (nfds > max_fdset)
>  		return -EINVAL;
> 
>  	poll_initwait(&table);

http://www.opengroup.org/onlinepubs/009695399/functions/poll.html sayeth

[EINVAL]
    The nfds argument is greater than {OPEN_MAX}, or ...

and afaict, max_fdset can be either less than or greater than OPEN_MAX,
depending upon how one sets NR_OPEN.

So I think that patch should be s/&&/||/ and s/changelog/new one/

If anything we do here alters userspace-visible behaviour (and it probably will)
then it should be *exahustively* documented in the changelog, and probably dropped.

Re: [PATCH] Fix poll() nfds check.

[Vadim Lobanov]

On Wed, 5 Jul 2006, Andrew Morton wrote:

> On Wed, 5 Jul 2006 20:00:34 -0700 (PDT)
> Vadim Lobanov <vlobanov@speakeasy.net> wrote:
>
> > diff -Npru linux-2.6.17-git25/fs/select.c linux-new/fs/select.c
> > --- linux-2.6.17-git25/fs/select.c	2006-07-05 19:06:56.000000000 -0700
> > +++ linux-new/fs/select.c	2006-07-05 19:10:51.000000000 -0700
> > @@ -671,7 +671,7 @@ int do_sys_poll(struct pollfd __user *uf
> >  	fdt = files_fdtable(current->files);
> >  	max_fdset = fdt->max_fdset;
> >  	rcu_read_unlock();
> > -	if (nfds > max_fdset && nfds > OPEN_MAX)
> > +	if (nfds > max_fdset)
> >  		return -EINVAL;
> >
> >  	poll_initwait(&table);
>
> http://www.opengroup.org/onlinepubs/009695399/functions/poll.html sayeth
>
> [EINVAL]
>     The nfds argument is greater than {OPEN_MAX}, or ...
>
> and afaict, max_fdset can be either less than or greater than OPEN_MAX,
> depending upon how one sets NR_OPEN.

Very good point. My manpages have led me astray to the dark side. :)

> So I think that patch should be s/&&/||/ and s/changelog/new one/

Yikes, I think sticking the OR operator in there would seriously suck,
given that programs would then be unable to poll on more than 256 file
descriptors (which actually happens, or so I've heard). Besides, it
would also kill Tigran's work, as per the comment from the top of
select.c:

012  *  24 January 2000
013  *     Changed sys_poll()/do_poll() to use PAGE_SIZE chunk-based allocation
014  *     of fds to overcome nfds < 16390 descriptors limit (Tigran Aivazian).
015  */

> If anything we do here alters userspace-visible behaviour (and it probably will)
> then it should be *exahustively* documented in the changelog, and probably dropped.

Arguments of the cited standard's sensibilities notwithstanding (given
that it seems silly to limit the API in unnatural ways, as we can easily
and provably handle more than 256 fds), it seems better to leave this as
is. Thanks for the critical eye. :)

- Vadim Lobanov

Re: [PATCH] Fix poll() nfds check.

[Ulrich Drepper]

On 7/5/06, Andrew Morton <akpm@osdl.org> wrote:
> [EINVAL]
>     The nfds argument is greater than {OPEN_MAX}, or ...

This requirement must be treated the same way as the EMFILE error in
open(): ignore the OPEN_MAX limit if ulimit says so.  The question is
what to do if the ulimit < OPEN_MAX.  POSIX does not require OPEN_MAX
to be the exact limit.

So, I think removing the OPEN_MAX comparison is the correct way to do
this here.  If somebody wants strict POSIX compliance they have to set
ulimit -n to 256.

Re: [PATCH] Fix poll() nfds check.

[Vadim Lobanov]

On Thu, 6 Jul 2006, Ulrich Drepper wrote:

> On 7/5/06, Andrew Morton <akpm@osdl.org> wrote:
> > [EINVAL]
> >     The nfds argument is greater than {OPEN_MAX}, or ...
>
> This requirement must be treated the same way as the EMFILE error in
> open(): ignore the OPEN_MAX limit if ulimit says so.  The question is
> what to do if the ulimit < OPEN_MAX.  POSIX does not require OPEN_MAX
> to be the exact limit.

This interpretation makes more sense to me. No hardcoded magic number
limits where the behavior of the syscall changes.

> So, I think removing the OPEN_MAX comparison is the correct way to do
> this here.  If somebody wants strict POSIX compliance they have to set
> ulimit -n to 256.

Andrew, assuming that you're willing to make this change to the code, I
can redo this patch against the latest rc and resend, if that'll make it
easier to apply.

-- Vadim Lobanov

Re: [PATCH] Fix poll() nfds check.

[H. Peter Anvin]

Vadim Lobanov wrote:
> Hi,
> 
> This is a trivial patch to fix the nfds check in the poll system call
> implementation. Namely, OPEN_MAX no longer does anything important in
> the kernel, and checking that nfds is greater than max_fdset AND greater
> than OPEN_MAX therefore just seems wrong.
> 
> This brings up a slightly-tangential question: Why do the nfds checks
> exist in select()/poll()? They're not strictly necessary, since bad
> input will be caught later when we validate all the fds, one by one.
> Furthermore, these checks optimize the handling of error cases (which
> should be uncommon) while pessimizing correct usage of the syscalls
> (which should be more common).
> 

The reason for these is presumably to keep applications which uses 
select() to overflow their fd_sets.  Unfortunately fd_set is defined in 
such a way that it's a static size.

Using ulimit seems like a reasonable compromise for this.

	-hpa