[patch 0/6] sys_indirect RFC - sys_indirect introduction

[patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

This patch-set implements the skeleton for a new sys_indirect() system call.
The reason for such system call would be to avoid the proliferation of new
system calls, introduced to only wrap old ones by setting/unsetting a given
context before/after the real system call.
The internal operation of sys_indirect() would be (pseudo-code):

	for_each_ctx(wrap_ctx, &user_ctxs) {
		save_prev_ctx(&saved_chain, tsk->xxx);
		tsk->xxx = wrap_ctx;
	}
	err = call_system_call(nr, params);
	for_each_saved_ctx(ctx, &saved_chain) {
		restore_ctx(tsk->xxx, ctx);
	}
	return err;

To made sys_indirect() decently flexible, and to avoid a sys_indirect2()
anytime soon, a simple "flags" parameter is clearly not sufficent to be
passed down with it.
To be flexible we need to allow 1) more than a simple "flags" to be passed
down 2) more than one context possibly to be set before the real system
call.  Also, the data structures passed down as new-context-to-be-set should
be compat-free, in order to simplify the compat code.
The prototype for sys_indirect() is:

	struct indirect_ctx {
		__u32 ctx;
	};

	long sys_indirect(unsigned int nr,
			const struct indirect_ctx **ctxs,
			unsigned int nctxs,
			const unsigned long *params);

The "struct indirect_ctx" is a stub structure that is supposed to be the
base for all the context set/unset operations.
An example usage for userspace POV (referring to the example in the next
patches) is:

[include/linux/indirect.h]
	#define SYSIND_CTX_OPENFLAGS	0
	struct sysind_ctx_OPENFLAGS {
		__u32 ctx;
		__u32 flags;
	};

[userspace]
	struct sysind_ctx_OPENFLAGS octx;
	struct indirect_ctx *ctxs[1];
	unsigned long params[6];
	
	octx.ctx = SYSIND_CTX_OPENFLAGS;
	octx.flags = O_CLOEXEC;
	ctxs[0] = (struct indirect_ctx *) &octx;
	params[0] = domain;
	params[1] = type;
	params[2] = proto;
	res = indirect(__NR_socket, ctxs, 1, params);


The sys_indirect() system call requires a small arch-dependent asm function
call_syscall() in order to call a system call by passing call number and
parameters (compat_call_syscall() is also needed for archs having 32 bit compat).
I currently implemented them for i386 and x86-64.
The following patches builds but are totally untested at the moment.
Comments?




- Davide

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Ulrich Drepper]

On 6/29/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> [include/linux/indirect.h]
>         #define SYSIND_CTX_OPENFLAGS    0
>         struct sysind_ctx_OPENFLAGS {
>                 __u32 ctx;
>                 __u32 flags;

I agree that this interface is more than any other in danger of
needing an interface change.  But I think your solution is a bit too
expensive and complex.  You need two reads from userlevel.

The standard way to handle this is a versioned struct.  I.e., define a
struct for the current needs, define an initial version.  To use the
syscall pass the version number and the struct pointer to the syscall.
 If the kernel doesn't know the version number it fails.  Otherwise it
might have to read old versions of the struct which is trivial to do.
E.g.:

#define SYSIND_VERSION 1
#define SYSIND_CTX_OPENFLAGS 0
#define SYSIND_CTX_SIGMASK 1
struct sysind_ctx {
  int ctx;
  union {
    int flags;
    kernel_sigset_t  sset;
  };
};

long sys_indirect(unsigned int nr,
                       int ctx_version,
                       const struct indirect_ctx *ctx,
                       const unsigned long *params);


This reduces the number of accesses to userlevel data to one and still
has all the flexibility needed.

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

On Fri, 29 Jun 2007, Ulrich Drepper wrote:

> On 6/29/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> > [include/linux/indirect.h]
> >         #define SYSIND_CTX_OPENFLAGS    0
> >         struct sysind_ctx_OPENFLAGS {
> >                 __u32 ctx;
> >                 __u32 flags;
> 
> I agree that this interface is more than any other in danger of
> needing an interface change.  But I think your solution is a bit too
> expensive and complex.  You need two reads from userlevel.

But, a __get_user(), once you scrap off all the gcc wrapping, is bacially 
a move. That could even be removed, but really I don't see the reason 
since it allows for a cleaner strcture definition in userland.




> The standard way to handle this is a versioned struct.  I.e., define a
> struct for the current needs, define an initial version.  To use the
> syscall pass the version number and the struct pointer to the syscall.
> If the kernel doesn't know the version number it fails.  Otherwise it
> might have to read old versions of the struct which is trivial to do.
> E.g.:
> 
> #define SYSIND_VERSION 1
> #define SYSIND_CTX_OPENFLAGS 0
> #define SYSIND_CTX_SIGMASK 1
> struct sysind_ctx {
>  int ctx;
>  union {
>    int flags;
>    kernel_sigset_t  sset;
>  };
> };

But this does not allow more than one context to be set at a time, like 
the current implementation does. Ie, the current implementation allow you 
to:

#define SYSIND_CTX_OPENFLAGS    0
struct sysind_ctx_OPENFLAGS {
	__u32 ctx;
	__u32 flags;
};

#define SYSIND_CTX_SIGSET       1
struct sysind_ctx_SIGSET {
	__u32 ctx;
	__compat_sigset set;
};

struct sysind_ctx_OPENFLAGS octx;
struct sysind_ctx_SIGSET sctx;
struct indirect_ctx *ctxs[2];
unsigned long params[6];

octx.ctx = SYSIND_CTX_OPENFLAGS;
octx.flags = O_CLOEXEC;
sctx.ctx = SYSIND_CTX_SIGSET;
sctx.set = SIG_XYZ | SIG_ABC;
ctxs[0] = (struct indirect_ctx *) &octx;
ctxs[1] = (struct indirect_ctx *) &sctx;
params[0] = blah;
params[1] = blew;
...
res = indirect(__NR_xxxx, ctxs, 2, params);


So you basically keep the context strcture separated, and allow to pass 
down more then one context to be set.
Another solution would be to have:

struct sysind_ctx_OPENFLAGS {
	__u32 flags;
};
struct sysind_ctx_SIGSET {
	__compat_sigset set;
};
struct monster_struct {
	__u32 size;
	__u64 flags;
	struct sysind_ctx_OPENFLAGS octx;
	struct sysind_ctx_SIGSET sctx;
	...
};

Where size gives you the size of the monster structure, and every bit on 
flags tells you which strcture inside monster is valid.
But I'd clearly prefer the former.



- Davide

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Ulrich Drepper]

On 6/30/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> But, a __get_user(), once you scrap off all the gcc wrapping, is bacially
> a move. That could even be removed, but really I don't see the reason
> since it allows for a cleaner strcture definition in userland.

Don't generalize.  The 4G/4G kernel, for instance, doesn't have this
property.  Who knows what else will come up.  Anyway, two memcpy are
slower than one.


> But this does not allow more than one context to be set at a time, like
> the current implementation does. Ie, the current implementation allow you
> to:

Of course it does.  It just requires an appropriate union element.
I've listed flags and sigset_t as separate union elements since I
cannot think of a place where we need both extensions.  Should there
be one this can easily be changed.

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

On Sat, 30 Jun 2007, Ulrich Drepper wrote:

> On 6/30/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> > But, a __get_user(), once you scrap off all the gcc wrapping, is bacially
> > a move. That could even be removed, but really I don't see the reason
> > since it allows for a cleaner strcture definition in userland.
> 
> Don't generalize.  The 4G/4G kernel, for instance, doesn't have this
> property.  Who knows what else will come up.  Anyway, two memcpy are
> slower than one.
>
> > But this does not allow more than one context to be set at a time, like
> > the current implementation does. Ie, the current implementation allow you
> > to:
> 
> Of course it does.  It just requires an appropriate union element.
> I've listed flags and sigset_t as separate union elements since I
> cannot think of a place where we need both extensions.  Should there
> be one this can easily be changed.

Why do you want to stick everything inside a structure, with nested 
strctures and unions, when they clearly are separate contexts?
Also, unions don't work if you want to pass multiple contexts at a time.
You need your structure to contains *all* the possible contexts.
You do not want to sell the monster-struct-union by branding an extra 
__get_user(), do you?
On top of that, the single pointer to the compat multi-context strcture, 
will force the kernel to decide which one of the members is actually valid 
or not. The set/unset callbacks separate each context from a structure POV 
and from a setup/teardown POV, and allow the kernel to use their native 
data structures. Think about if we had this for the latest 
pselect/ppoll/epoll_pwait. Think about how your solution and mine would 
apply to that very much concrete case.




- Davide

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

On Sat, 30 Jun 2007, Davide Libenzi wrote:

> Think about if we had this for the latest pselect/ppoll/epoll_pwait. 
> Think about how your solution and mine would apply to that very much 
> concrete case.

This is how all those overloaded syscalls looks like, BTW:

        if (sigmask) {
                if (sigsetsize != sizeof(sigset_t))
                        return -EINVAL;
                if (copy_from_user(&ksigmask, sigmask, sizeof(ksigmask)))
                        return -EFAULT;
                sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
                sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
        }
        error = sys_XXX(...);
        if (sigmask) {
                if (error == -EINTR) {
                        memcpy(&current->saved_sigmask, &sigsaved,
                               sizeof(sigsaved));
                        set_thread_flag(TIF_RESTORE_SIGMASK);
                } else
                        sigprocmask(SIG_SETMASK, &sigsaved, NULL);
        }

How would you do that with a single shared strcture, w/out adding in all 
signal paths the knowledge of the structure?



- Davide

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Ulrich Drepper]

On 6/30/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> This is how all those overloaded syscalls looks like, BTW:
> [...]
> How would you do that with a single shared strcture, w/out adding in all
> signal paths the knowledge of the structure?

You said it yourself: each individual wrapper would look like this.
Generalization really isn't possible, you'll have each wrapper syscall
looking different.  This means there is no reason to try coming up
with some overly complicated data structure which would only be useful
if the processing of that data structure could be centralized.

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

On Sun, 1 Jul 2007, Ulrich Drepper wrote:

> On 6/30/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> > This is how all those overloaded syscalls looks like, BTW:
> > [...]
> > How would you do that with a single shared strcture, w/out adding in all
> > signal paths the knowledge of the structure?
> 
> You said it yourself: each individual wrapper would look like this.
> Generalization really isn't possible, you'll have each wrapper syscall
> looking different.  This means there is no reason to try coming up
> with some overly complicated data structure which would only be useful
> if the processing of that data structure could be centralized.

With the current API design you'd able to easily confine the "pre" code 
inside the "set" function, and the "post" code inside the "unset" 
function. It looks pretty clean to me, and allows to limit the knowledge 
of sys_indirect, the more as possible inside kernel/indirect.c.



- Davide

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Ulrich Drepper]

On 7/1/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> With the current API design you'd able to easily confine the "pre" code
> inside the "set" function, and the "post" code inside the "unset"
> function. It looks pretty clean to me, and allows to limit the knowledge
> of sys_indirect, the more as possible inside kernel/indirect.c.

But this will not be applicable.  We already discussed that each
syscall likely needs its own set of flags etc.  There really isn't
much overlap if any which cannot be handled at least as well using a
flat structure.  You're adding major complications for something which
IMO will never be usable.  With the flat structure to whole overhead
of sys_indirect is limited to a test for valid syscalls, copying the
struct, making the call to the syscall function, and resetting the
value in current.  Very simple and fast.

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

On Mon, 2 Jul 2007, Ulrich Drepper wrote:

> On 7/1/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> > With the current API design you'd able to easily confine the "pre" code
> > inside the "set" function, and the "post" code inside the "unset"
> > function. It looks pretty clean to me, and allows to limit the knowledge
> > of sys_indirect, the more as possible inside kernel/indirect.c.
> 
> But this will not be applicable.  We already discussed that each
> syscall likely needs its own set of flags etc.  There really isn't
> much overlap if any which cannot be handled at least as well using a
> flat structure.  You're adding major complications for something which
> IMO will never be usable.  With the flat structure to whole overhead
> of sys_indirect is limited to a test for valid syscalls, copying the
> struct, making the call to the syscall function, and resetting the
> value in current.  Very simple and fast.

Never be usable? I made you a concrete example that is like 8 months old. 
And *that* could not have been cleanly handled with the flat structure 
idea.
The extra flags parameter is one example where we'd need an extra flags 
field in the task_struct in any case. So you need in any case code that 
does extra checks and merges normal parameters/flags with the shared 
context ones. This independently of the method used. But there are 
examples (and the signal stuff is one of them), where you do need the 
set_context+syscall+unset_context abstraction, for all cases where the 
kernel already has its own internal data strctures. In those cases you'd 
have to spread sys_internal context knowledge all around the kernel, 
whereas the current solution allows you to confine the code inside 
kernel/indirect.c (through the set/unset abstraction). And this w/out even 
try to hit the weak spot of about how this structure will look after a few 
additions.



- Davide

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Ulrich Drepper]

On 7/2/07, Davide Libenzi <davidel@xmailserver.org> wrote:
> Never be usable? I made you a concrete example that is like 8 months old.
> And *that* could not have been cleanly handled with the flat structure
> idea.

First of all, sigmasks are not widely needed.  Second, why on earth
shouldn't it be possible with a flat structure?  Just have the sigmask
in the same position.  It's no magic.


> But there are
> examples (and the signal stuff is one of them), where you do need the
> set_context+syscall+unset_context abstraction, for all cases where the
> kernel already has its own internal data strctures. In those cases you'd
> have to spread sys_internal context knowledge all around the kernel,
> whereas the current solution allows you to confine the code inside
> kernel/indirect.c

Nonsense.  Whether you have a new word in the task structure or a
pointer to a structure, you have to embed knowledge of this
indirection in every affected code paths.  There is no difference
here.  The only difference is that trying to force everything through
an artificially complicated common entry code.

I hope that Andrew+Linus will see through this.  I'm done arguing.

Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

[Davide Libenzi]

On Mon, 2 Jul 2007, Ulrich Drepper wrote:

> > But there are
> > examples (and the signal stuff is one of them), where you do need the
> > set_context+syscall+unset_context abstraction, for all cases where the
> > kernel already has its own internal data strctures. In those cases you'd
> > have to spread sys_internal context knowledge all around the kernel,
> > whereas the current solution allows you to confine the code inside
> > kernel/indirect.c
> 
> Nonsense.  Whether you have a new word in the task structure or a
> pointer to a structure, you have to embed knowledge of this
> indirection in every affected code paths.  There is no difference
> here.  The only difference is that trying to force everything through
> an artificially complicated common entry code.
> 
> I hope that Andrew+Linus will see through this.  I'm done arguing.

I simply asked you to look how the code/patch would look like to handle 
the ppoll/pselect/epoll_pwait stuff, and precisely where your changes had 
to go. Or even in the exmaple Linus made of "make the fsuid/fsgid 
temporarily be my _real_ uid/gid for this single system call" for what it 
matters.



- Davide