Extend sys_clone and sys_unshare system calls API
Subsystems (4)
PATCH 1/2 Extend sys_clone and sys_unshare system calls API
There's only one bit in the clone_flags left, so we won't be able to create more namespaces after we make it busy. Besides, for checkpoint/restart jobs we might want to create tasks with given pids (virtual of course). And nobody knows for sure what else might be required from clone() in the future.
This is an attempt to create a extendable API for clone and unshare. Actually this patch is a request for comment about the overall design. If it will turn out to "look good", then we'll select some better names for new flag and data types.
I use the last bit in the clone_flags for CLONE_LONGARG. When set it will denote that the child_tidptr is not a pointer to a tid storage, but the pointer to the struct long_clone_struct which currently looks like this:
struct long_clone_arg {
int size;
};When we want to add a new argument for clone we just put it *at the end of this structure* and adjust the size. The binary compatibility with older long_clone_arg-s is facilitated with the helper macro clone_arg_has(). It checks, that the desired member is provided from the userspace.
Cedric implemented the same thing for unshare - he added the second argument for it and iff the CLONE_NEWCLONE is specified - uses it. Binary compatibility with the old ushare will be kept.
The new argument is to be pulled up to the create_new_namespaces, which is done in a second patch.
This patch is made against the 2.6.24-rc6-mm1. It is tested to pass more flags into create_new_namespaces.
---
include/linux/sched.h +22 -0
@@ -27,6 +27,23 @@#define CLONE_NEWUSER 0x10000000/* New user namespace */ #define CLONE_NEWPID 0x20000000/* New pid namespace */ #define CLONE_NEWNET 0x40000000/* New network namespace */ +#define CLONE_LONGARG 0x80000000/* Has an long_clone_arg */ + +/* +* If the CLONE_LONGARG argument is specified, then the +* child_tidptr is expected to point to the struct long_clone_arg. +* +* This structure has a variable size, which is to be recorded +* in the size member. All other members a to be put after this. +* Never ... No. Always add new members only at its tail. +* +* The clone_arg_has() macro is responsible for checking whether +* the given arg has the desired member. +*/ + +struct long_clone_arg { +int size; +};/* * Scheduling policies
@@ -40,6 +57,11 @@#ifdef __KERNEL__ +#define clone_arg_has (arg, member) ({ \ + struct long_clone_arg *__carg = arg; \ + (__carg->size >= offsetof(struct long_clone_arg, member) + \ + sizeof(__carg->member)); }) +struct sched_param {int sched_priority; };
include/linux/syscalls.h +1 -1
@@ -585,7 +585,7 @@ asmlinkage long compat_sys_newfstatat(unsigned int dfd, char __user * filename, int flag);asmlinkage long compat_sys_openat (unsigned int dfd,const char __user *filename,int flags,int mode); -asmlinkage long sys_unshare (unsigned long unshare_flags); +asmlinkage long sys_unshare (unsigned long unshare_flags,int __user *flagptr); asmlinkage long sys_splice(int fd_in, loff_t __user *off_in, int fd_out, loff_t __user *off_out,
kernel/fork.c +54 -4
116 lines not on this page · raw message
PATCH 2/2 Propagate the long_clone_arg up to the create_new_namespaces
The first user for the long_clone_arg is the namespaces code, so pull the extended argument up to the namespaces creation function.
---
include/linux/nsproxy.h +3 -2
@@ -62,12 +62,13 @@ static inline struct nsproxy *task_nsproxy(struct task_struct *tsk)return rcu_dereference (tsk->nsproxy); } -int copy_namespaces (unsigned long flags,struct task_struct *tsk); +int copy_namespaces (unsigned long flags,struct task_struct *tsk, +struct long_clone_arg *carg);void exit_task_namespaces (struct task_struct *tsk);void switch_task_namespaces (struct task_struct *tsk,struct nsproxy *new);void free_nsproxy (struct nsproxy *ns);int unshare_nsproxy_namespaces (unsigned long ,struct nsproxy **, -struct fs_struct *); +struct fs_struct *,struct long_clone_arg *carg);static inline void put_nsproxy (struct nsproxy *ns) {
kernel/fork.c +2 -2
@@ -1160,7 +1199,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, goto bad_fork_cleanup_signal;if ((retval =copy_keys (clone_flags, p))) goto bad_fork_cleanup_mm; -if ((retval =copy_namespaces (clone_flags, p))) +if ((retval =copy_namespaces (clone_flags, p, carg))) goto bad_fork_cleanup_keys; retval =copy_thread (0 , clone_flags, stack_start, stack_size, p, regs);if (retval)
@@ -1705,7 +1753,7 @@ asmlinkage long sys_unshare(unsigned long unshare_flags)if ((err =unshare_semundo (unshare_flags, &new_ulist))) goto bad_unshare_cleanup_fd;if ((err =unshare_nsproxy_namespaces (unshare_flags, &new_nsproxy, - new_fs))) + new_fs, carg))) goto bad_unshare_cleanup_semundo;if (new_fs || new_mm || new_fd || new_ulist || new_nsproxy) {
kernel/nsproxy.c +11 -7
@@ -48,7 +48,8 @@ static inline struct nsproxy *clone_nsproxy(struct nsproxy *orig)* leave it to the caller to do proper locking and attach it to task. */ static struct nsproxy *create_new_namespaces (unsigned long flags, -struct task_struct *tsk,struct fs_struct *new_fs) +struct task_struct *tsk,struct fs_struct *new_fs, +struct long_clone_arg *carg) {struct nsproxy *new_nsp;int err;
@@ -119,7 +120,8 @@ out_ns:* called from clone. This now handles copy for nsproxy and all * namespaces therein. */ -int copy_namespaces (unsigned long flags,struct task_struct *tsk) +int copy_namespaces (unsigned long flags,struct task_struct *tsk, +struct long_clone_arg *carg) {struct nsproxy *old_ns = tsk->nsproxy;struct nsproxy *new_ns;
@@ -131,7 +133,8 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)get_nsproxy (old_ns);if (!(flags & (CLONE_NEWNS |CLONE_NEWUTS |CLONE_NEWIPC | -CLONE_NEWUSER |CLONE_NEWPID |CLONE_NEWNET ))) +CLONE_NEWUSER |CLONE_NEWPID |CLONE_NEWNET | +CLONE_LONGARG )))return 0 ;if (!capable (CAP_SYS_ADMIN )) {
@@ -139,7 +142,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk) goto out; } - new_ns =create_new_namespaces (flags, tsk, tsk->fs); + new_ns =create_new_namespaces (flags, tsk, tsk->fs, carg);if (IS_ERR (new_ns)) { err =PTR_ERR (new_ns); goto out;
@@ -179,19 +182,20 @@ void free_nsproxy(struct nsproxy *ns)* On success, returns the new nsproxy. */ int unshare_nsproxy_namespaces (unsigned long unshare_flags, -struct nsproxy **new_nsp,struct fs_struct *new_fs) +struct nsproxy **new_nsp,struct fs_struct *new_fs, +struct long_clone_arg *carg) {int err =0 ;if (!(unshare_flags & (CLONE_NEWNS |CLONE_NEWUTS |CLONE_NEWIPC | -CLONE_NEWUSER |CLONE_NEWNET ))) +CLONE_NEWUSER |CLONE_NEWNET |CLONE_LONGARG )))return 0 ;if (!capable (CAP_SYS_ADMIN ))return -EPERM ; *new_nsp =create_new_namespaces (unshare_flags, current, - new_fs ? new_fs : current->fs); + new_fs ? new_fs : current->fs, carg);if (IS_ERR (*new_nsp)) { err =PTR_ERR (*new_nsp); goto out;
Hi, Pavel,
[Adding Ulrich]
Quoted 4 lines
I use the last bit in the
clone_flagsforCLONE_LONGARG. When set it will denote that thechild_tidptris not a pointer to a tid storage, but the pointer to the structlong_clone_structwhich currently looks like this:
I'm probably just totally off the deep end, but something did occur to
me: this looks an awful lot like a special version of the sys_indirect() idea. Unless it has been somehow decided that sys_indirect() is the wrong idea, might it not be better to look at making that interface solve the extended clone() problem as well?
jon
Quoted 9 lines · Jonathan Corbet
Hi, Pavel,
[Adding Ulrich]
Quoted 4 lines
I use the last bit in the
clone_flagsforCLONE_LONGARG. When set it will denote that thechild_tidptris not a pointer to a tid storage, but the pointer to the structlong_clone_structwhich currently looks like this:
Quoted 5 lines
I'm probably just totally off the deep end, but something did occur to
me: this looks an awful lot like a special version of thesys_indirect()idea. Unless it has been somehow decided thatsys_indirect()is the wrong idea, might it not be better to look at making that interface solve the extendedclone()problem as well?
We has such an idea, but as far as I know sys_indirect idea is not yet accomplished. So I risked to propose such an extension for sys_clone.
If Andrew says that sys_indirect is on its way to -mm tree, then we'll surely wait and use this one for namespaces.
Quoted 2 lines
jon
Thanks,
Pavel
Quoted 9 lines · Jonathan Corbet
Hi, Pavel,
[Adding Ulrich]
Quoted 4 lines
I use the last bit in the
clone_flagsforCLONE_LONGARG. When set it will denote that thechild_tidptris not a pointer to a tid storage, but the pointer to the structlong_clone_structwhich currently looks like this:
Quoted 5 lines
I'm probably just totally off the deep end, but something did occur to
me: this looks an awful lot like a special version of thesys_indirect()idea. Unless it has been somehow decided thatsys_indirect()is the wrong idea, might it not be better to look at making that interface solve the extendedclone()problem as well?
Nah, just put an XML parser into the kernel to have the form match the contents...
Al "perhaps we should newgroup alt.tasteless.api for all that stuff" Viro
Quoted 15 lines · Al Viro
Quoted 8 lines · Jonathan Corbet
Hi, Pavel,
[Adding Ulrich]
Quoted 4 lines
I use the last bit in the
clone_flagsforCLONE_LONGARG. When set it will denote that thechild_tidptris not a pointer to a tid storage, but the pointer to the structlong_clone_structwhich currently looks like this:Quoted 5 lines
I'm probably just totally off the deep end, but something did occur to
me: this looks an awful lot like a special version of thesys_indirect()idea. Unless it has been somehow decided thatsys_indirect()is the wrong idea, might it not be better to look at making that interface solve the extendedclone()problem as well?
Quoted 4 lines
Nah, just put an XML parser into the kernel to have the form match the contents...
Al "perhaps we should newgroup alt.tasteless.api for all that stuff" Viro
so you'd rather have new syscalls to support new clone flags ? something like :
long sys_clone64(unsigned long flags_high, unsigned long flag_low) long sys_unshare64(unsigned long flags_high, unsigned long flag_low)
Thanks,
C.
Extending syscalls (was: [PATCH 1/2] Extend sys_clone and sys_unshare system calls API)
In reply to Al ViroAl Viro sez:
Quoted 4 lines
Nah, just put an XML parser into the kernel to have the form match the contents...
Al "perhaps we should newgroup alt.tasteless.api for all that stuff" Viro
Heh, indeed. But we do seem to have a recurring problem of people wanting to extend sys_foo() beyond the confines of its original API. I've observed a few ways of doing that:
- create sys_foo2() (or sys_foo64(), or sys_fooat(), or sys_pfoo(), or...) and add the new stuff there.
- Put a version number into the API somewhere - wireless extensions, for example.
- Set a flag saying "I've stashed some additional parameters somewhere else." That's sys_indirect() and the current proposal for extending clone().
- Just do it all with a kernel-based XML parser. I think we should call this approach sys_viro() in honor of its champion.
- Do it all in sysfs
The first approach has traditionally been the most popular. If we have a consensus that this is the way to extend system calls in the future, it would be nice to set that down somewhere. We could avoid a lot of API blind alleys that way.
jon
Re: Extending syscalls
Quoted 5 lines · Jonathan Corbet
Heh, indeed. But we do seem to have a recurring problem of people wanting to extend
sys_foo()beyond the confines of its original API. I've observed a few ways of doing that:
Quoted 8 lines
- create
sys_foo2()(orsys_foo64(), orsys_fooat(), orsys_pfoo(), or...) and add the new stuff there.The first approach has traditionally been the most popular. If we have a consensus that this is the way to extend system calls in the future, it would be nice to set that down somewhere. We could avoid a lot of API blind alleys that way.
I would argue it is the right approach. It lets the kernel system call entry dispatch directly to the system call for the "new" case, and to a compatibility thunk for the "old" case. It has the following desirable properties:
- No overhead for the "new" case.
- Minimal overhead for the "old" case.
- Easily dealt with by tools like strace that examine system calls.
-hpa
Quoted 11 lines · Pavel Emelyanov
There's only one bit in the
clone_flagsleft, so we won't be able to create more namespaces after we make it busy. Besides, for checkpoint/restart jobs we might want to create tasks with given pids (virtual of course). And nobody knows for sure what else might be required fromclone()in the future.This is an attempt to create a extendable API for clone and unshare. Actually this patch is a request for comment about the overall design. If it will turn out to "look good", then we'll select some better names for new flag and data types.
Quoted 8 lines
I use the last bit in the
clone_flagsforCLONE_LONGARG. When set it will denote that thechild_tidptris not a pointer to a tid storage, but the pointer to the structlong_clone_structwhich currently looks like this:struct long_clone_arg { int size; };
Ugly as night, I'd say. (Al said it better). What about just adding clone2 syscall, that takes u64?
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 17 lines · Pavel Machek
Quoted 16 lines · Pavel Emelyanov
There's only one bit in the
clone_flagsleft, so we won't be able to create more namespaces after we make it busy. Besides, for checkpoint/restart jobs we might want to create tasks with given pids (virtual of course). And nobody knows for sure what else might be required fromclone()in the future.This is an attempt to create a extendable API for clone and unshare. Actually this patch is a request for comment about the overall design. If it will turn out to "look good", then we'll select some better names for new flag and data types.
I use the last bit in the
clone_flagsforCLONE_LONGARG. When set it will denote that thechild_tidptris not a pointer to a tid storage, but the pointer to the structlong_clone_structwhich currently looks like this:
Quoted 6 lines
Quoted 3 lines
struct long_clone_arg { int size; };Ugly as night, I'd say. (Al said it better). What about just adding clone2 syscall, that takes u64?
yes but we would need more something like :
long sys_clone64(unsigned long flags_high, unsigned long flag_low)
if we want the syscall to be supported on 32bit arch. clone2 is also being used on ia64 already.
C.
Quoted 6 lines · Cedric Le Goater
yes but we would need more something like :
long
sys_clone64(unsigned long flags_high, unsigned long flag_low)if we want the syscall to be supported on 32bit arch. clone2 is also being used on ia64 already.
Did we decide not to do something with a variable number of arguments?
sys_clonefoo(unsigned long *flags, int len);
-- Dave
Quoted 11 lines · Dave Hansen
Quoted 6 lines · Cedric Le Goater
yes but we would need more something like :
long
sys_clone64(unsigned long flags_high, unsigned long flag_low)if we want the syscall to be supported on 32bit arch. clone2 is also being used on ia64 already.
Did we decide not to do something with a variable number of arguments?
sys_clonefoo(unsigned long *flags, int len);
That is evil, because that means strace can no longer reliably print flags being used (for example).
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 7 lines · Pavel Machek
Quoted 1 line
Did we decide not to do something with a variable number of
arguments?
Quoted 2 lines
sys_clonefoo(unsigned long *flags, int len);That is evil, because that means strace can no longer reliably print flags being used (for example).
Ahhh. Just like it can't print strings for "buf"?
write(int fd, char *buf, size_t len)
Man, strace is stupid! ;)
If that's *really* a concern, why don't we just pass, say 4 or 5 longs in:
sys_clonebig(unsigned long flags0, unsigned long flags1, unsigned long flags2, unsigned long flags3);
-- Dave
Quoted 6 lines · Dave Hansen
Quoted 5 lines · Pavel Machek
Quoted 1 line
Did we decide not to do something with a variable number of
arguments?
Quoted 2 lines
sys_clonefoo(unsigned long *flags, int len);
Quoted 8 lines
Quoted 2 lines
That is evil, because that means strace can no longer reliably print flags being used (for example).
Ahhh. Just like it can't print strings for "buf"?
write(int fd, char *buf, size_t len)Man, strace is stupid! ;)
I said _reliably_.
Guess what happens on smp when one process will periodically overwrite buf, and second will do the syscall?
It sucks for write, but it would suck much more for clone, where flags drastically change the behaviour.
(Yep, it is possible to do reliably, see subterfugue, but it is a lot of work and lot of overhead).
Quoted 5 lines
If that's *really* a concern, why don't we just pass, say 4 or 5 longs in:
sys_clonebig(unsigned long flags0, unsigned long flags1, unsigned long flags2, unsigned long flags3);
Works for me.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html