Marking select(2) as restrict

[Dimitry Andric]

On 28 Feb 2018, at 10:40, Gary Jennejohn <gljennjohn at gmail.com> wrote:
> 
> On Tue, 27 Feb 2018 14:30:24 -0800 (PST)
> Chris Torek <torek at elf.torek.net> wrote:
> 
>> If we can back up a bit, there are (at least) two mostly-separate
>> issues:
>> 
>> * What types and qualfiiers to use for various parameters:
>>   These are dictated by standards, and we should just conform
>>   to whatever is in the standard unless there's some major
>>   reason to do otherwise.
>> 
>>   POSIX says that we *should* use "restrict" here:
>>   http://pubs.opengroup.org/onlinepubs/009696699/functions/pselect.html
>> 
>>   so if no one has a major reason to do otherwise (such as
>>   "no conforming program can tell that we didn't and all it will
>>   achieve is nothing" :-) ) I'd say add the restrict.
>> 
>>   (As kib and others have noted, it achieves a lot of nothing, so
>>   the priority for this particular conformance seems low.)
>> 
>> * Select itself: it makes no sense *in the caller* to pass
>>   the same &fd_set argument to more than one parameter unless
>>   they are all input-only.  The kernel is going to write on them,
>>   and there is no guarantee that the kernel will write them in any
>>   particular order.  Hence if you write:
>> 
>>        ret = select(nfds, &a, &a, &a, timeout);
>> 
>>   you are nearly guaranteed to have a bug: this call can only
>>   be correct if the only value you intend to inspect is "ret".
>> 
>>   (That, in fact, is what devd was doing: if ret==0, daemonize,
>>   and in any case throw away the returned results in the by-pointer
>>   arguments.  Hence not a bug -- unless ... well, read on.)
>> 
>>   The remaining question is: can even that be correct?  In
>>   particular, should the kernel be required to use copy-in/copy-out
>>   semantics, or can it do the equivalent (minus error checking
>>   and a lot of other important details) of:
>> 
>>        int select(int nfds, fd_set *in, fd_set *out, fd_set *ex,
>>            struct timeval *tvp) {
>>                some_complicated_type v_in, v_out, v_ex;
>> 
>>                v1 = op(*in);
>>                *in = function(v1);
>>                v2 = op(*out);
>>                ...
>>        }
>> 
>>   ?  If the latter is *permitted*, then the call passing "&a"
>>   three times is *always* wrong, since *in might have been
>>   clobbered before *out is ever loaded.  If a program breaks
>>   because it was doing this, well, it was already broken, we were
>>   just lucky that it worked anyway.  (Was this good luck, or bad
>>   luck? :-) )
>> 
>>   Currently, the kernel does in fact copy them all in,
>>   then futz with them, then copy them all out (for good reasons).
>>   So the devd usage always works.  But will the kernel *always*
>>   do this, or is that an accident of the implementation?
>> 
> 
> Linux also does a copyin (copy_from_user) of each FD_SET.  Linux
> also does not use restrict in select.

Not in the kernel, where it is effectively declared as:

asmlinkage long sys_select(int n, fd_set __user *inp, fd_set __user *outp,
                       	fd_set __user *exp, struct timeval __user *tvp);

but definitely in userland, where glibc has:

extern int __select (int __nfds, fd_set *__restrict __readfds,
		     fd_set *__restrict __writefds,
		     fd_set *__restrict __exceptfds,
		     struct timeval *__restrict __timeout);

So for any normal program the entry point to select() is certainly
using restrict.


> It would be a very bad
> design decision to not copy each FD_SET separately into the
> kernel.

Indeed, Linux's sys_select allocates 6 separate bitmaps, 3 for the
incoming fd_sets, and 3 for the outgoing ones.

If the outgoing ones would all use the same pointer, it is just going
to end up calling copy_to_user() 3 times on the same area, overwriting
the previous results.  That would still be unsafe for some situations.


> It would be good to explicitly state that it is only safe to
> use pointers to a single FD_SET when only the error return is of
> interest and the results in the FD_SET will be ignored.

Agreed.

-Dimitry

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 223 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20180228/8bc3d575/attachment.sig>