Messages in this thread | | | Date | Mon, 14 Apr 2008 09:58:40 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM |
| |
Quoting Manfred Spraul (manfred@colorfullife.com): > Andrew Morton wrote: >> On Sun, 13 Apr 2008 10:04:17 +0200 Manfred Spraul >> <manfred@colorfullife.com> wrote: >> >> >>> sys_unshare(CLONE_NEWIPC) doesn't handle the undo lists properly, this >>> can >>> cause a kernel memory corruption. CLONE_NEWIPC must detach from the >>> existing >>> undo lists. >>> Fix, part 1: add support for sys_unshare(CLONE_SYSVSEM) >>> >>> >> >> Is this a non-back-compatible change? >> >> > It adds a new feature - previously sys_unshare(CLONE_SYSVSEM) returned > -EINVAL. > >>> Signed-off-by: Manfred Spraul <manfred@colorfullife.com> >>> --- >>> ipc/sem.c | 1 + >>> kernel/fork.c | 18 ++++++++++++++---- >>> 2 files changed, 15 insertions(+), 4 deletions(-) >>> >>> diff --git a/ipc/sem.c b/ipc/sem.c >>> index 0b45a4d..35841bd 100644 >>> --- a/ipc/sem.c >>> +++ b/ipc/sem.c >>> @@ -1298,6 +1298,7 @@ void exit_sem(struct task_struct *tsk) >>> undo_list = tsk->sysvsem.undo_list; >>> if (!undo_list) >>> return; >>> + tsk->sysvsem.undo_list = NULL; >>> if (!atomic_dec_and_test(&undo_list->refcnt)) >>> return; >>> diff --git a/kernel/fork.c b/kernel/fork.c >>> index 9c042f9..7f242b0 100644 >>> --- a/kernel/fork.c >>> +++ b/kernel/fork.c >>> @@ -1675,13 +1675,17 @@ static int unshare_fd(unsigned long >>> unshare_flags, struct files_struct **new_fdp >>> } >>> /* >>> - * Unsharing of semundo for tasks created with CLONE_SYSVSEM is not >>> - * supported yet >>> + * Unsharing of semundo for tasks created with CLONE_SYSVSEM doesn't >>> require >>> + * any allocations: it means that the task leaves the existing undo >>> lists, >>> + * just like sys_exit(). The new undo lists are allocated on demand in >>> the >>> + * ipc syscalls. >>> + * new_ulistp is set to a non-NULL value, the caller expects that on >>> success. >>> */ >>> static int unshare_semundo(unsigned long unshare_flags, struct >>> sem_undo_list **new_ulistp) >>> { >>> - if (unshare_flags & CLONE_SYSVSEM) >>> - return -EINVAL; >>> + if (unshare_flags & CLONE_SYSVSEM) { >>> + *new_ulistp = (void*)1; >>> + } >>> >> >> And can we do anything nicer than this? >> >> > Attached is an alternative. If you prefer it, I'll send another patch set.
FWIW I definately far far prefer this version :)
As for 'maintainers', in the end wrt this code I'd defer to any two of Pavel, Nadia, and Pierre, each of who I've seen do a great deal of digging around this code.
(I think I saw some set of these go into -mm so I guess I'll just grab mmotm and test with that a bit later today)
thanks, -serge
> -- > Manfred
> diff --git a/ipc/sem.c b/ipc/sem.c > index 0b45a4d..35841bd 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -1298,6 +1298,7 @@ void exit_sem(struct task_struct *tsk) > undo_list = tsk->sysvsem.undo_list; > if (!undo_list) > return; > + tsk->sysvsem.undo_list = NULL; > > if (!atomic_dec_and_test(&undo_list->refcnt)) > return; > diff --git a/kernel/fork.c b/kernel/fork.c > index 9c042f9..535aa92 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1675,18 +1675,6 @@ static int unshare_fd(unsigned long unshare_flags, struct files_struct **new_fdp > } > > /* > - * Unsharing of semundo for tasks created with CLONE_SYSVSEM is not > - * supported yet > - */ > -static int unshare_semundo(unsigned long unshare_flags, struct sem_undo_list **new_ulistp) > -{ > - if (unshare_flags & CLONE_SYSVSEM) > - return -EINVAL; > - > - return 0; > -} > - > -/* > * unshare allows a process to 'unshare' part of the process > * context which was originally shared using clone. copy_* > * functions used by do_fork() cannot be used here directly > @@ -1701,7 +1689,6 @@ asmlinkage long sys_unshare(unsigned long unshare_flags) > struct sighand_struct *new_sigh = NULL; > struct mm_struct *mm, *new_mm = NULL, *active_mm = NULL; > struct files_struct *fd, *new_fd = NULL; > - struct sem_undo_list *new_ulist = NULL; > struct nsproxy *new_nsproxy = NULL; > > check_unshare_flags(&unshare_flags); > @@ -1724,13 +1711,17 @@ asmlinkage long sys_unshare(unsigned long unshare_flags) > goto bad_unshare_cleanup_sigh; > if ((err = unshare_fd(unshare_flags, &new_fd))) > goto bad_unshare_cleanup_vm; > - if ((err = unshare_semundo(unshare_flags, &new_ulist))) > - goto bad_unshare_cleanup_fd; > if ((err = unshare_nsproxy_namespaces(unshare_flags, &new_nsproxy, > new_fs))) > - goto bad_unshare_cleanup_semundo; > + goto bad_unshare_cleanup_fd; > > - if (new_fs || new_mm || new_fd || new_ulist || new_nsproxy) { > + if (new_fs || new_mm || new_fd || (unshare_flags & CLONE_SYSVSEM) || new_nsproxy) { > + if (unshare_flags & CLONE_SYSVSEM) { > + /* > + * CLONE_SYSVSEM is equivalent to sys_exit(). > + */ > + exit_sem(current); > + } > > if (new_nsproxy) { > switch_task_namespaces(current, new_nsproxy); > @@ -1766,7 +1757,6 @@ asmlinkage long sys_unshare(unsigned long unshare_flags) > if (new_nsproxy) > put_nsproxy(new_nsproxy); > > -bad_unshare_cleanup_semundo: > bad_unshare_cleanup_fd: > if (new_fd) > put_files_struct(new_fd);
| |