* [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
@ 2008-04-13 8:04 Manfred Spraul
2008-04-13 8:59 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: Manfred Spraul @ 2008-04-13 8:04 UTC (permalink / raw
To: Andrew Morton
Cc: Linux Kernel Mailing List, Serge E. Hallyn, Eric W. Biederman,
Pavel Emelyanov, Sukadev Bhattiprolu
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)
I'm not sure who's the right maintainer, Andrew, could you merge it?
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;
+ }
return 0;
}
@@ -1731,6 +1735,12 @@ asmlinkage long sys_unshare(unsigned long unshare_flags)
goto bad_unshare_cleanup_semundo;
if (new_fs || new_mm || new_fd || new_ulist || 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);
--
1.5.4.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-13 8:04 [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM Manfred Spraul
@ 2008-04-13 8:59 ` Andrew Morton
2008-04-13 11:36 ` Manfred Spraul
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2008-04-13 8:59 UTC (permalink / raw
To: Manfred Spraul
Cc: Linux Kernel Mailing List, Serge E. Hallyn, Eric W. Biederman,
Pavel Emelyanov, Sukadev Bhattiprolu
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?
>
> 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?
> return 0;
> }
> @@ -1731,6 +1735,12 @@ asmlinkage long sys_unshare(unsigned long unshare_flags)
> goto bad_unshare_cleanup_semundo;
>
> if (new_fs || new_mm || new_fd || new_ulist || 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);
> --
> 1.5.4.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-13 8:59 ` Andrew Morton
@ 2008-04-13 11:36 ` Manfred Spraul
2008-04-13 18:16 ` Andrew Morton
2008-04-14 14:58 ` Serge E. Hallyn
0 siblings, 2 replies; 8+ messages in thread
From: Manfred Spraul @ 2008-04-13 11:36 UTC (permalink / raw
To: Andrew Morton
Cc: Linux Kernel Mailing List, Serge E. Hallyn, Eric W. Biederman,
Pavel Emelyanov, Sukadev Bhattiprolu
[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]
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.
--
Manfred
[-- Attachment #2: patch-step1-alternative --]
[-- Type: text/plain, Size: 2366 bytes --]
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);
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-13 11:36 ` Manfred Spraul
@ 2008-04-13 18:16 ` Andrew Morton
2008-04-14 14:58 ` Serge E. Hallyn
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2008-04-13 18:16 UTC (permalink / raw
To: Manfred Spraul
Cc: Linux Kernel Mailing List, Serge E. Hallyn, Eric W. Biederman,
Pavel Emelyanov, Sukadev Bhattiprolu
On Sun, 13 Apr 2008 13:36:24 +0200 Manfred Spraul <manfred@colorfullife.com> wrote:
> >> + 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.
Well I suppose we can fiddle with this sort of thing later. Right now the
major concern is the testedness and reviewedness of these changes?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-13 11:36 ` Manfred Spraul
2008-04-13 18:16 ` Andrew Morton
@ 2008-04-14 14:58 ` Serge E. Hallyn
2008-04-14 19:39 ` Andrew Morton
2008-04-14 21:44 ` Serge E. Hallyn
1 sibling, 2 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2008-04-14 14:58 UTC (permalink / raw
To: Manfred Spraul
Cc: Andrew Morton, Linux Kernel Mailing List, Serge E. Hallyn,
Eric W. Biederman, Pavel Emelyanov, Sukadev Bhattiprolu,
Pierre PEIFFER
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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-14 14:58 ` Serge E. Hallyn
@ 2008-04-14 19:39 ` Andrew Morton
2008-04-14 21:18 ` Serge E. Hallyn
2008-04-14 21:44 ` Serge E. Hallyn
1 sibling, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2008-04-14 19:39 UTC (permalink / raw
To: Serge E. Hallyn
Cc: manfred, linux-kernel, serue, ebiederm, xemul, sukadev,
pierre.peiffer
On Mon, 14 Apr 2008 09:58:40 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:
> Quoting Manfred Spraul (manfred@colorfullife.com):
> > Andrew Morton wrote:
> >> On Sun, 13 Apr 2008 10:04:17 +0200 Manfred Spraul
> >> <manfred@colorfullife.com> wrote:
> >>
> >>> 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 :)
Oh, OK.
I guess I'll drop what I have. Manfred, can we please have a new patchset?
We might end up slipping this back to 2.6.25.1. Would that be a bad thing?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-14 19:39 ` Andrew Morton
@ 2008-04-14 21:18 ` Serge E. Hallyn
0 siblings, 0 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2008-04-14 21:18 UTC (permalink / raw
To: Andrew Morton
Cc: Serge E. Hallyn, manfred, linux-kernel, ebiederm, xemul, sukadev
Quoting Andrew Morton (akpm@linux-foundation.org):
> On Mon, 14 Apr 2008 09:58:40 -0500
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
>
> > Quoting Manfred Spraul (manfred@colorfullife.com):
> > > Andrew Morton wrote:
> > >> On Sun, 13 Apr 2008 10:04:17 +0200 Manfred Spraul
> > >> <manfred@colorfullife.com> wrote:
> > >>
> > >>> 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 :)
>
> Oh, OK.
>
> I guess I'll drop what I have. Manfred, can we please have a new patchset?
>
> We might end up slipping this back to 2.6.25.1. Would that be a bad thing?
It'd be unfortunate, but as the existing patch doesn't completely fix
the problem it's probably best to wait anyway.
Manfred, I don't want to step on your toes, but please do let me know if
you don't have time to do the next version. If you do have time, then
thank you again for spotting+fixing the problem.
thanks,
-serge
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM
2008-04-14 14:58 ` Serge E. Hallyn
2008-04-14 19:39 ` Andrew Morton
@ 2008-04-14 21:44 ` Serge E. Hallyn
1 sibling, 0 replies; 8+ messages in thread
From: Serge E. Hallyn @ 2008-04-14 21:44 UTC (permalink / raw
To: Serge E. Hallyn
Cc: Manfred Spraul, Andrew Morton, Linux Kernel Mailing List,
Eric W. Biederman, Pavel Emelyanov, Sukadev Bhattiprolu,
Pierre PEIFFER, Michael Kerrisk
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-04-14 21:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-04-13 8:04 [PATCH 1/2] fix sys_unshare()+SEM_UNDO: add support for CLONE_SYSVSEM Manfred Spraul
2008-04-13 8:59 ` Andrew Morton
2008-04-13 11:36 ` Manfred Spraul
2008-04-13 18:16 ` Andrew Morton
2008-04-14 14:58 ` Serge E. Hallyn
2008-04-14 19:39 ` Andrew Morton
2008-04-14 21:18 ` Serge E. Hallyn
2008-04-14 21:44 ` Serge E. Hallyn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.