Messages in this thread | | | Date | Mon, 14 Apr 2008 14:23:26 -0500 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH 2/2] fix sys_unshare()+SEM_UNDO: perform an implicit CLONE_SYSVSEM in CLONE_NEWIPC |
| |
Quoting Manfred Spraul (manfred@colorfullife.com): > Serge E. Hallyn wrote: >> Quoting Manfred Spraul (manfred@colorfullife.com): >> >>> 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 2: perform an implicit CLONE_SYSVSEM in CLONE_NEWIPC. >>> CLONE_NEWIPC creates a new IPC namespace, the task cannot access the >>> existing semaphore arrays after the unshare syscall. Thus the task >>> can/must detach from the existing undo list entries, too. >>> >>> This fixes the kernel corruption, because it makes it impossible that >>> undo records from two different namespaces are in sysvsem.undo_list. >>> >> >> So this was never an issue with clone(CLONE_NEWIPC|CLONE_SYSVSEM), which >> should have had the same result as unshare(CLONE_NEWIPC&~CLONE_SYSVSEM)? >> >> > Actually, the story is slightly different: > unshare(CLONE_NEWIPC|CLONE_SYSVSEM) returns -EINVAL right now.
Right, but it's feasible with clone (where CLONE_SYSVSEM has the opposite meaning of with unshare) right? According to the comment, CLONE_SYSVSEM was disabled mainly out of fear that it would be confusing to enable it since it does have the inverse meaning in unshare as in clone.
> Thus all apps right now call unshare(CLONE_NEWIPC|&~CLONE_SYSVSEM). > This combination doesn't make much sense. Even worse - it easily causes a > kernel oops. > Thus my fix is twofold: > - add support for unshare(CLONE_SYSVSEM). > - implicitely add CLONE_SYSVSEM to all calls that set CLONE_NEWIPC.
Right but your fix ignores the fact that you can achieve the same result as unshare(CLONE_NEWIPC&~CLONE_SYSVSEM) by doing clone(CLONE_NEWIPC|CLONE_SYSVSEM).
> It's not really pretty: If a pivot_namespace syscall is ever added, then > CLONE_NEWIPC&~CLONE_SYSVSEM would make sense again. > What do you think? Can we break backward compatibility and add > if ( (unshare_flags & CLONE_NEWIPC) && !(unshare_flags & CLONE_SYSVSEM) > ) > return -EINVAL; > into sys_unshare()?
I don't think we need to, I think what you're doing makes perfect sense. If you asked for CLONE_NEWIPC, then you wanted to do CLONE_SYSVSEM (uh, or, in clone parlance, !CLONE_SYSVSEM :)
> I have decided against that, it breaks the current ABI. > And we gain virtually nothing - most if not all unshare users will be > single threaded apps that do not use sysvsem at all, and even most sysvsem > users do not use SEM_UNDO.
And most importantly sharing your semundo list but not your sems with your parent is silly!
-serge
| |