Messages in this thread | | | Date | Mon, 14 Apr 2008 16:44:17 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM |
| |
Quoting Serge E. Hallyn (serue@us.ibm.com): > 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
Of course if we do go this route, then the unshare(2) manpage will need an update (so Michael Kerrisk cc:d).
thanks, -serge
| |