Re: [PATCHv3 0/4] sys_indirect system call

[Eric Dumazet <dada1@cosmosbay.com>]

On Mon, 19 Nov 2007 07:12:29 -0800
Ulrich Drepper <drepper@redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Eric Dumazet wrote:
> >>   union indirect_params i;
> >>   i.file_flags.flags = O_CLOEXEC;
> > 
> > This setup forbids future addons to file_flags
> > 
> > In three years, when we want to add a new indirect feature to socket() 
> > call, do we need a new indirect2() syscall ?
> 
> No, it doesn't.  The setup is indefinitely expandable.
> 
> All you need to do, if it becomes necessary to have more than an int, is
> to define a little structure for the system call and then use it.  The
> only requirement is that the code has to assume a value of zero is what
> is used today.  That's the whole point.

Yes, this is what I understood.

> 
> union indirect_params {
>   struct {
>     int flags;
>   } file_flags;
>   struct {
>     int flags;
>     int new_syscall_data1;
>     sigset_t and_a_sigmask;
>   } new_data;
> };

Yes, so now, if you take this new definition of indirect_params,
its size is 12 bytes at least.

So when you recompile your old program (as you post it and as I commented on),
it will pass a >= 12 bytes data to kernel, with only first 4 bytes set to O_CLOEXEC.

Other bytes will contain junk (automatic variables are not set to 0 by compiler), 
and your program possibly break - if kernel socket() call was
extended to use new_data struct. (ie wanting O_CLOEXEC features +
 new ones with new_syscall_data1)

You have to take care of binary compatibility, but also that an old program
 (unaware of the new fields) will be re-compiled with new headers.

> 
> Old programs will set only the 'flags' member of 'new_data' while new
> once can also set the new elements.  New programs on old kernels will
> eithe have failing calls since the structure is too big or the call will
> not have all the desired effects.  The latter can be tested for.
> 
> 
> > Or better, you could avoid using 'union indirect_params' in user code, and 
> > only use the substructs for each function.
> 
> There is no overhead introduced through the union.  The only reason the
> union is there in the first place is to allocate sufficient data in
> task_struct to cover all cases.

Yes, sure, but it should be a kernel issue, not a user space one at all,
as long as user provides the size of the data he want to pass to kernel (
and your patch does this already)