Re: [PATCHv3 0/4] sys_indirect system call

Eric Dumazet
2007-11-19T15:43:39Z

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)