fix SEM_UNDO with namespaces
Subsystems (3)
RFC PATCH 1/1 fix SEM_UNDO with namespaces
Hi,
the attached patch should fix the combination of CLONE_NEWIPC with shared sysv undo structures (the common case, just sys_unshare(CLONE_NEWIPC)):lookup_undo() now locates the undo array based on both semid and the namespace pointer.
Additionally, the patch tries to clean the code up by using the linked list macros from <linux/list.h> instead of single linked lists.
The patch passes a few quick tests, I'm interested in feedback. Are there test apps for testing the IPC namespace code?
--
Manfred
include/linux/sem.h +7 -6
@@ -95,7 +95,7 @@ struct sem_array {struct sem *sem_base;/* ptr to first semaphore in array */ struct sem_queue *sem_pending;/* pending operations to be processed */ struct sem_queue **sem_pending_last;/* last pending operation */ -struct sem_undo *undo;/* undo requests on this array */ +struct list_head list_id;/* undo requests on this array */ unsigned long sem_nsems;/* no. of semaphores in array */ };
@@ -118,9 +118,10 @@ struct sem_queue {* when the process exits. */ struct sem_undo { -struct sem_undo * proc_next;/* next entry on this process */ -struct sem_undo * id_next;/* next entry on this semaphore set */ +struct list_head list_proc;/* per-process list: all undos from one process */ +struct list_head list_id;/* per semaphore array list: all undos for one array */ int semid;/* semaphore set identifier */ +struct ipc_namespace *ns;/* namespace */ short * semadj;/* array of adjustments, one per semaphore */ };
@@ -128,9 +129,9 @@ struct sem_undo {* that may be shared among all a CLONE_SYSVSEM task group. */ struct sem_undo_list { -atomic_t refcnt; -spinlock_t lock; -struct sem_undo *proc_list; +atomic_t refcnt; +spinlock_t lock; +struct list_head list_proc; };struct sysv_sem {
ipc/sem.c +100 -94
307 lines not on this page · raw message
Quoted 7 lines · Manfred Spraul
Hi,
the attached patch should fix the combination of
CLONE_NEWIPCwith shared sysv undo structures (the common case, justsys_unshare(CLONE_NEWIPC)):lookup_undo()now locates the undo array based on both semid and the namespace pointer.
If you start using any IPC object and then call unshare with CLONE_NEWIPC, then it's your problem, but not the kernel. There are many issues that can become broken from the user-level POV.
What's the problem with undo list? Does it become irrelevant and no undos happen after the task dies?
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
Quoted 2 lines
Additionally, the patch tries to clean the code up by using the linked list macros from <
linux/list.h> instead of single linked lists.
May I ask you to split the patch into atomic parts, rather than mixing fixes, reworks and cleanups together?
Quoted 2 lines
The patch passes a few quick tests, I'm interested in feedback. Are there test apps for testing the IPC namespace code?
This code is a part of OpenVZ kernels, so it passes some internal tests we perform.
Quoted 3 lines
--
Manfred
Thanks,
Pavel
Quoted 11 lines · Pavel Emelyanov
Quoted 5 lines · Manfred Spraul
Hi,
the attached patch should fix the combination of
CLONE_NEWIPCwith shared sysv undo structures (the common case, justsys_unshare(CLONE_NEWIPC)):lookup_undo()now locates the undo array based on both semid and the namespace pointer.If you start using any IPC object and then call unshare with
CLONE_NEWIPC, then it's your problem, but not the kernel.
The result is a kernel memory corruption, and kernel memory corruptions are always the kernel's problem.
The code assumed that a semaphore id is globally unique. With namespaces, this is not true anymore.
If two semaphore arrays exist with the same id, but different sizes, then semops will cause memory corruptions: The undo structure contains one element for each semaphore, thus the semop will write behind the end of the memory allocation.
Quoted 2 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
A very tricky question: Let's assume we have a process with two threads.
The undo structure is shared, as per opengroup standard.
Now one thread calls unshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it. If we allow sys_unshare() for multithreaded processes with CLONE_NEWIPC and without CLONE_SYSVSEM, then we must handle this case.
--
Manfred
Quoted 11 lines · Manfred Spraul
Quoted 10 lines · Pavel Emelyanov
Quoted 8 lines · Manfred Spraul
Hi,
the attached patch should fix the combination of
CLONE_NEWIPCwith shared sysv undo structures (the common case, justsys_unshare(CLONE_NEWIPC)):lookup_undo()now locates the undo array based on both semid and the namespace pointer.
Quoted 5 lines
Quoted 3 lines
If you start using any IPC object and then call unshare with
CLONE_NEWIPC, then it's your problem, but not the kernel.The result is a kernel memory corruption, and kernel memory corruptions are always the kernel's problem.
Agree. Must be fixed, but I'm not sure we should try handling this case by trying to de-op semaphores for former task namespace. I think that destroying this list or returning -EBUSY for this case is OK.
Quoted 10 lines
The code assumed that a semaphore id is globally unique. With namespaces, this is not true anymore.
If two semaphore arrays exist with the same id, but different sizes, then semops will cause memory corruptions: The undo structure contains one element for each semaphore, thus the semop will write behind the end of the memory allocation.Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
Quoted 7 lines
A very tricky question: Let's assume we have a process with two threads. The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it.
If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.
Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
Quoted 3 lines
--
Manfred
Quoted 40 lines · Pavel Emelyanov
Quoted 11 lines · Manfred Spraul
Quoted 10 lines · Pavel Emelyanov
Quoted 8 lines · Manfred Spraul
Hi,
the attached patch should fix the combination of
CLONE_NEWIPCwith shared sysv undo structures (the common case, justsys_unshare(CLONE_NEWIPC)):lookup_undo()now locates the undo array based on both semid and the namespace pointer.Quoted 5 lines
Quoted 3 lines
If you start using any IPC object and then call unshare with
CLONE_NEWIPC, then it's your problem, but not the kernel.The result is a kernel memory corruption, and kernel memory corruptions are always the kernel's problem.
Agree. Must be fixed, but I'm not sure we should try handling this case by trying to de-op semaphores for former task namespace. I think that destroying this list or returning
-EBUSYfor this case is OK.Quoted 10 lines
The code assumed that a semaphore id is globally unique. With namespaces, this is not true anymore.
If two semaphore arrays exist with the same id, but different sizes, then semops will cause memory corruptions: The undo structure contains one element for each semaphore, thus the semop will write behind the end of the memory allocation.Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
Quoted 7 lines
A very tricky question: Let's assume we have a process with two threads. The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it.
If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.
Quoted 3 lines
Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
Absolutely.
-serge
Andrew Morton
maintainerQuoted 41 lines · On Tue, 1 Apr 2008 09:15:41 -0500 "Serge E. Hallyn" <[email protected]> wrote:
Quoted 40 lines · Pavel Emelyanov
Quoted 11 lines · Manfred Spraul
Quoted 10 lines · Pavel Emelyanov
Quoted 8 lines · Manfred Spraul
Hi,
the attached patch should fix the combination of
CLONE_NEWIPCwith shared sysv undo structures (the common case, justsys_unshare(CLONE_NEWIPC)):lookup_undo()now locates the undo array based on both semid and the namespace pointer.Quoted 5 lines
Quoted 3 lines
If you start using any IPC object and then call unshare with
CLONE_NEWIPC, then it's your problem, but not the kernel.The result is a kernel memory corruption, and kernel memory corruptions are always the kernel's problem.
Agree. Must be fixed, but I'm not sure we should try handling this case by trying to de-op semaphores for former task namespace. I think that destroying this list or returning
-EBUSYfor this case is OK.Quoted 17 lines
The code assumed that a semaphore id is globally unique. With namespaces, this is not true anymore.
If two semaphore arrays exist with the same id, but different sizes, then semops will cause memory corruptions: The undo structure contains one element for each semaphore, thus the semop will write behind the end of the memory allocation.Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
A very tricky question: Let's assume we have a process with two threads. The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it.
If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.
Quoted 6 lines
Quoted 3 lines
Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
Absolutely.
Guys, what's the status here?
afaict Manfred has identified an available-to-unprivileged-apps kernel memory corrupter? If so, we should fix it asap for 2.6.25. And for 2.6.24.x if it's also present there.
Manfred's patch doesn't come close to applying against the 2.6.26 IPC things which we have queued but that's OK - bugfixes come first.
Quoted 5 lines · Andrew Morton
Quoted 4 lines
Absolutely.
Quoted 7 lines
Guys, what's the status here?
afaict Manfred has identified an available-to-unprivileged-apps kernel memory corrupter? If so, we should fix it asap for 2.6.25. And for 2.6.24.x if it's also present there.
No, it's a priveledged-only memory corruption:
Quoted 11 lines
int unshare_nsproxy_namespaces(unsigned long
unshare_flags,
struct nsproxy **new_nsp, structfs_struct*new_fs){ int err = 0; [snip] if (!capable(CAP_SYS_ADMIN)) return -EPERM;Manfred's patch doesn't come close to applying against the 2.6.26 IPC things which we have queued but that's OK - bugfixes come first.
Where can I find the queued changes? Are they in -mm?
--
Manfred
Quoted 11 lines · Pavel Emelyanov
Quoted 10 lines
Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
A very tricky question: Let's assume we have a process with two threads. The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it.
If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.
Quoted 3 lines
Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
I almost agree. sys_unshare() in a multithreaded process breaks all kinds of user space libs. So you can only reasonably look at the problem as what we do with linux tasks that share some things and not others. The posix/opengroup notion of processes and threads are a distraction.
In this case requiring it appears that to require unsharing both CLONE_SYSVSEM and CLONE_NEWIPC at the same time. (i.e. unshare of CLONE_SYSVSEM should fail if CLONE_NEWIPC is not also specified).
Then to make it work we make unshare of SYSVSEM succeed when it is not shared.
This looks like about a 5 line patch or two.
The effect is because we don't support unsharing of SYSVSEM currently we don't support a threaded process unsharing the ipc namespace.
Eric
Quoted 21 lines · Eric W. Biederman
Quoted 12 lines · Pavel Emelyanov
Quoted 8 lines
Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
A very tricky question: Let's assume we have a process with two threads.
The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it. If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
I almost agree.
sys_unshare()in a multithreaded process breaks all kinds of user space libs. So you can only reasonably look at the problem as what we do with linux tasks that share some things and not others. The posix/opengroup notion of processes and threads are a distraction.
Quoted 8 lines
In this case requiring it appears that to require unsharing both
CLONE_SYSVSEMandCLONE_NEWIPCat the same time. (i.e. unshare ofCLONE_SYSVSEMshould fail ifCLONE_NEWIPCis not also specified). Then to make it work we make unshare of SYSVSEM succeed when it is not shared.This looks like about a 5 line patch or two.
Probably something like this in copy_ipcs:
if (current->sysvsem.undo_list != NULL &&
atomic_read(current->sysvsem.undo_list->refcount) > 1)
return -EINVAL;I'll think about it and write a patch over the weekend.
Quoted 3 lines
The effect is because we don't support unsharing of SYSVSEM currently we don't support a threaded process unsharing the ipc namespace.
I agree.
Btw: The manpage of unshare() is IMHO a bit misleading:
Quoted 2 lines
NAME
unshare - disassociate parts of the process execution context
unshare() only operates on a single thread, shouldn't the description be "disassociate parts of the thread execution context"?
--
Manfred
Quoted 29 lines · Eric W. Biederman
Quoted 11 lines · Pavel Emelyanov
Quoted 10 lines
Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
A very tricky question: Let's assume we have a process with two threads. The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it.
If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.Quoted 3 lines
Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
I almost agree.
sys_unshare()in a multithreaded process breaks all kinds of user space libs. So you can only reasonably look at the problem as what we do with linux tasks that share some things and not others. The posix/opengroup notion of processes and threads are a distraction.In this case requiring it appears that to require unsharing both
CLONE_SYSVSEMandCLONE_NEWIPCat the same time. (i.e. unshare ofCLONE_SYSVSEMshould fail ifCLONE_NEWIPCis not also specified).Then to make it work we make unshare of SYSVSEM succeed when it is not shared.
Quoted 6 lines
This looks like about a 5 line patch or two.
The effect is because we don't support unsharing of SYSVSEM currently we don't support a threaded process unsharing the ipc namespace.
Eric
Eric, does the following patch correctly interpret your recommendation?
Pavel does it make sense to you?
thanks,
-serge
Quoted 1 line
From
9c85fb3cb80cea1d888c3c253a9fb6e9bc173649Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <[email protected]>
Date: Thu, 3 Apr 2008 12:43:23 -0700
Subject: [PATCH 1/1] ipc namespaces: fix svsem unsharing issue
Refuse to unshare an ipcns if the semundo is shared and we are not requesting a new SYSVSEM
--- ipc/namespace.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
ipc/namespace.c +10 -0
@@ -48,6 +48,16 @@ struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns)if (!(flags &CLONE_NEWIPC ))return ns; +if (!(flags &CLONE_SYSVSEM )) { +if (!current->sysvsem.undo_list) + goto ok; +if (atomic_read (¤t->sysvsem.undo_list->refcnt) ==1 ) + goto ok; +put_ipc_ns (ns); +return ERR_PTR (-EINVAL ); + } + +ok: new_ns =clone_ipc_ns (ns);put_ipc_ns (ns);
Signature · 1 line
1.5.3.6
Quoted 62 lines · Serge E. Hallyn
Quoted 29 lines · Eric W. Biederman
Quoted 11 lines · Pavel Emelyanov
Quoted 10 lines
Quoted 3 lines
I agree, that we should probably destroy this one when the task calls unshare, but trying to keep this list relevant is useless.
A very tricky question: Let's assume we have a process with two threads. The undo structure is shared, as per opengroup standard.
Now one thread callsunshare(CLONE_NEWIPC). What should happen? We cannot destroy the undo structure, the other thread might be still interested in it.
If we allowsys_unshare()for multithreaded processes withCLONE_NEWIPCand withoutCLONE_SYSVSEM, then we must handle this case.Quoted 3 lines
Hm... I'd simply disable creating any new namespaces for threads. I think other namespaces developers agree with me. Serge, Suka, Eric what do you think?
I almost agree.
sys_unshare()in a multithreaded process breaks all kinds of user space libs. So you can only reasonably look at the problem as what we do with linux tasks that share some things and not others. The posix/opengroup notion of processes and threads are a distraction.In this case requiring it appears that to require unsharing both
CLONE_SYSVSEMandCLONE_NEWIPCat the same time. (i.e. unshare ofCLONE_SYSVSEMshould fail ifCLONE_NEWIPCis not also specified).Then to make it work we make unshare of SYSVSEM succeed when it is not shared.
Quoted 6 lines
This looks like about a 5 line patch or two.
The effect is because we don't support unsharing of SYSVSEM currently we don't support a threaded process unsharing the ipc namespace.
Eric
Eric, does the following patch correctly interpret your recommendation?
Pavel does it make sense to you?
thanks,
-sergeQuoted 1 line
From
9c85fb3cb80cea1d888c3c253a9fb6e9bc173649Mon Sep 17 00:00:00 2001From: Serge E. Hallyn <[email protected]>
Date: Thu, 3 Apr 2008 12:43:23 -0700
Subject: [PATCH 1/1] ipc namespaces: fix svsem unsharing issueRefuse to unshare an ipcns if the semundo is shared and we are not requesting a new SYSVSEM
Signed-off-by Serge E. Hallyn <[email protected]>--- ipc/namespace.c | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)ipc/namespace.c +0
-0
ipc/namespace.c struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns) · line 51
Quoted 4 lines
ipc/namespace.c +1
-0@@ -48,6 +48,16 @@ struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns)if (!(flags &CLONE_NEWIPC ))return ns; +if (!(flags &CLONE_SYSVSEM )) {
Wait, this should be opposite, right?
ipc/namespace.c struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns) · line 54
Quoted 3 lines
ipc/namespace.c +3
-0+if (!current->sysvsem.undo_list) + goto ok; +if (atomic_read (¤t->sysvsem.undo_list->refcnt) ==1 )
And the refcnt check really isn't needed, right? Either undo_list exists and both parent and child have it, or it doesn't and we're fine. So I suspect I can remove that.
Manfred, I'm trying to test this, but can't get an error without this patch. Do you have a testcase? (I've tried some combinations of creating a semarray in the new ipcns, creating one with fewer elements than the index on which the undo operation was done, and not creating a semarray in the new ipcsns at all, but the current code seems to do fine.
Or maybe I fundamentally misunderstand the bug.
Quoted 11 lines
ipc/namespace.c +6
-0+ goto ok; +put_ipc_ns (ns); +return ERR_PTR (-EINVAL ); + } + +ok: new_ns =clone_ipc_ns (ns);put_ipc_ns (ns);Signature · 1 line
1.5.3.6
Quoted 8 lines · Serge E. Hallyn
Quoted 8 lines
ipc/namespace.c +0
-0@@ -48,6 +48,16 @@ struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns)if (!(flags &CLONE_NEWIPC ))return ns;
Quoted 8 lines
ipc/namespace.c struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns) · line 52
Quoted 2 lines
ipc/namespace.c +1
-0+if (!(flags &CLONE_SYSVSEM )) {Wait, this should be opposite, right?
[snip]
Manfred, I'm trying to test this, but can't get an error without this patch. Do you have a testcase?
The patch is bogus, sorry that I didn't notice it immediately.
The problem is ipc/sem.c, function find_undo, lookup_undo:lookup_undo doesn't check the namespace pointer, thus a simple single threaded app can trigger the problem.
Attached is a test app and a kernel patch that shows the problem.
Run the test app immediately after boot (or within a new ipc namespace), otherwise the ipc sequence counter will prevent the app from triggering the problem: The undo structure that was created before unshare() [i.e. with 1 semaphore in it] will be used after unshare() [i.e. semaphore 100 will be accessed].
With kernel debugging (full slub debugging) enabled, I even got an oops when I tried to ipcrm the left over array after running undons, probably because the undo structure was freed at exit_sem() within the new namespace, but still used in the outer namespace.
--
Manfred
/*
* Copyright (C) 1999,2001 by Manfred Spraul.
*
* Redistribution of this file is permitted under the terms of the GNU
* General Public License (GPL)
* $Header: /home/manfred/cvs-tree/manfred/ipcsem/undotest.c,v 1.2 2003/06/28 15:19:43 manfred Exp $ */
#ifdef __LINUX__ #define _GNU_SOURCE #include <sched.h> #include <wait.h> #define CLONE_NEWIPC 0x08000000 /* New ipcs */
#endif
#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/sem.h>
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <assert.h>
#include <signal.h>
#include <time.h>
#include <unistd.h>
#include <pthread.h>
union semun {
int val;
struct semid_ds *buf;
unsigned short int *array;
struct seminfo* __buf;
};int getval(char * str, int id)
{
union semun arg;
int res;
res = semctl(id,0,GETVAL,arg);
if(res==-1) {
printf("GETVAL failed for %s.\n", str);
exit(4);
}
printf(" %s: GETVAL now %d.\n",str, res);
return res;
}void setzero(int id)
{
union semun arg;
int res;
arg.val = 0;
res = semctl(id,0,SETVAL,arg);
if(res==-1) {
printf("SETVAL failed, errno %d.\n", errno);
exit(4);
}
printf(" SETVAL succeeded.\n");
}/* test1: verify that undo works at all. */
int main(int argc,char** argv)
{
int res, id;
printf(" ****************************************\n");
/* create array */
res = semget(IPC_PRIVATE, 1, 0700 | IPC_CREAT);
printf(" got semaphore array %xh.\n",res);
if(res == -1) {
printf(" create failed.\n");
return 1;
}
id = res;
setzero(id);
res = getval("test1 init", id);
if (res != 0) {
printf("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n");
printf("Bad: got unexpected value.\n");
exit(99);
}
/* create sub-process */
res = fork();
if (res < 0) {
printf("Fork failed (errno=%d). Aborting.\n", errno);
res = semctl(id,1,IPC_RMID,NULL);
exit(1);
}
fflush(stdout);
if (!res) {
struct sembuf sop[1];
int id2;
sop[0].sem_num=0;
sop[0].sem_op=1;
sop[0].sem_flg=SEM_UNDO;
res = semop(id,sop,1);
if(res==-1) {
printf("semop failed.\n");
exit(1);
}
res = getval("before unshare()", id);
if (res != 1) {
printf("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n");
printf("Bad: got unexpected value.\n");
exit(99);
}
fflush(stdout);
errno = 0;
res = unshare(CLONE_NEWNS|CLONE_NEWIPC);
printf(" unshare returned %d, errno now %d (expected: 0,0).\n", res, errno);
fflush(stdout);
/* create array */
res = semget(IPC_PRIVATE, 101, 0700 | IPC_CREAT);
printf(" got semaphore array %xh.\n",res);
if(res == -1) {
printf(" create failed.\n");
return 1;
}
id2 = res;
printf("Now operating on id1=%x, id2=%x. (if both are equal, there will be a memory corruption)\n", id, id2);
/* child: */
sop[0].sem_num=100;
sop[0].sem_op=1;
sop[0].sem_flg=SEM_UNDO;
// This operation should trash kernel memory (if id1==id2)
res = semop(id2,sop,1);
if(res==-1) {
printf("semop failed.\n");
exit(1);
}
exit(1);
} else {
int retval;
retval = wait4(res, NULL, 0, NULL);
if (retval != res) {
printf("wait4 returned unexpeted value %d, errno now %d.\n",
retval, errno);
}
res = getval("after exit", id);
if (res != 0) {
printf("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n");
printf("Bad: got unexpected value.\n");
return 1;
}
}
return 0;
}ipc/sem.c +2 -0
@@ -1081,6 +1082,7 @@ static struct sem_undo *find_undo(struct ipc_namespace *ns, int semid) new->id_next = sma->undo; sma->undo = new;sem_unlock (sma); +printk (KERN_ERR "find_undo(%p, %x) new undop %p (for %d sems).\n" , ns, semid, new, nsems); un = new;spin_unlock (&ulp->lock); out:
@@ -1153,6 +1155,7 @@ retry_undos: error =PTR_ERR (sma); goto out_free; } +printk (KERN_ERR "find_undo(%p, %x) == %p, using %ld sems.\n" , ns, semid, un, sma->sem_nsems);/* * semid identifiers are not unique - find_undo may have
PATCH 1/1 fix SEM_UNDO with namespaces, take 2
Hi,
below is the second attempt to fix SEM_UNDO + unshare():lookup_undo (in ipc/sem.c) is not namespace-aware, thus all entries in sysvsem.undo_list must be from the same namespace. The patch enforces that by detaching the current thread from sysvsem.undo_list in switch_task_namespaces() if the ipc namespace is changed.
The patch boots and passes simple sysvsem+unshare tests.
ipc/sem.c +1 -0
@@ -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 ;
kernel/nsproxy.c +12 -0
@@ -211,6 +211,18 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)might_sleep (); +if ((p->nsproxy ==NULL && new !=NULL ) || + (p->nsproxy !=NULL && new ==NULL ) || + (p->nsproxy !=NULL && new !=NULL && p->nsproxy->ipc_ns != new->ipc_ns)) { +/* switching the IPC namespace is considered equivalent to sys_exit() wrt. +* to outstanding SEM_UNDO undos: After switching to the new IPC namespace, +* the semaphore arrays from the old namespace are not accessible anymore. +* +* Additionally, an implicit sys_unshare(CLONE_SYSVSEM) is performed. +*/ +exit_sem (p); + } + ns = p->nsproxy;rcu_assign_pointer (p->nsproxy, new);
Re: [PATCH] fix SEM_UNDO with namespaces, take 2
Quoted 19 lines · Manfred Spraul
Hi,
below is the second attempt to fix
SEM_UNDO+unshare():lookup_undo(inipc/sem.c) is not namespace-aware, thus all entries in sysvsem.undo_list must be from the same namespace.
The patch enforces that by detaching the current thread from sysvsem.undo_list inswitch_task_namespaces()if the ipc namespace is changed.The patch boots and passes simple sysvsem+unshare tests.
Signed-off-by Manfred Spraul <[email protected]>kernel/nsproxy.c +2
-0@@ -211,6 +211,18 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)might_sleep (); + if ((p->nsproxy == NULL && new != NULL) || + (p->nsproxy != NULL && new == NULL) ||
kernel/nsproxy.c void switch_task_namespaces(struct task_struct *p, struct nsproxy *new) · line 225
Quoted 10 lines
kernel/nsproxy.c +10
-0+ (p->nsproxy != NULL && new != NULL && p->nsproxy->ipc_ns != new->ipc_ns)) { +/* switching the IPC namespace is considered equivalent to sys_exit() wrt. +* to outstanding SEM_UNDO undos: After switching to the new IPC namespace, +* the semaphore arrays from the old namespace are not accessible anymore. +* +* Additionally, an implicit sys_unshare(CLONE_SYSVSEM) is performed. +*/ +exit_sem (p); + } +
No, switch_task_namespaces is the wrong place to do this. It is to be done in copy_ipc_ns. If you need a task for which a new namespace is being prepared, then pass one into it.
Quoted 3 lines
kernel/nsproxy.c +0
-0ns = p->nsproxy;rcu_assign_pointer (p->nsproxy, new);
Re: [PATCH] fix SEM_UNDO with namespaces, take 2
Quoted 3 lines · Pavel Emelyanov
No,
switch_task_namespacesis the wrong place to do this. It is to be done incopy_ipc_ns. If you need a task for which a new namespace is being prepared, then pass one into it.
copy_ipc_ns() is also the wrong place:
There are calls after copy_ipc_ns() that can fail (e.g. copy_pid_ns()), and if they fail, then the whole syscall is aborted. But undoing outstanding semaphore operations cannot be undone.
Or simpler: the copy_whatever() functions do not modify current.
Another option would be within sys_unshare():sys_unshare() first creates all new pointers, and then actual unsharing is performed.
What do you think? I that the right place? The implementation could be moved into a seperate function, perhaps some of the NULL tests are superflous, too.
--
Manfred
ipc/sem.c +1 -0
@@ -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 ;
kernel/fork.c +12 -0
@@ -1733,6 +1733,18 @@ asmlinkage long sys_unshare(unsigned long unshare_flags)if (new_fs || new_mm || new_fd || new_ulist || new_nsproxy) {if (new_nsproxy) { +if ((current->nsproxy ==NULL && new_nsproxy !=NULL ) || + (current->nsproxy !=NULL && new_nsproxy ==NULL ) || + (current->nsproxy !=NULL && new_nsproxy !=NULL && current->nsproxy->ipc_ns != new_nsproxy->ipc_ns)) { +/* switching the IPC namespace is considered equivalent to sys_exit() wrt. +* to outstanding SEM_UNDO undos: After switching to the new IPC namespace, +* the semaphore arrays from the old namespace are not accessible anymore. +* +* Additionally, an implicit sys_unshare(CLONE_SYSVSEM) is performed. +*/ +exit_sem (current); + } +switch_task_namespaces (current, new_nsproxy); new_nsproxy =NULL ; }
Re: [PATCH] fix SEM_UNDO with namespaces, take 2
kernel/fork.c asmlinkage long sys_unshare(unsigned long unshare_flags) · line 1738
Quoted 3 lines
kernel/fork.c +3
-0+if ((current->nsproxy ==NULL && new_nsproxy !=NULL ) || + (current->nsproxy !=NULL && new_nsproxy ==NULL ) || + (current->nsproxy !=NULL && new_nsproxy !=NULL && current->nsproxy->ipc_ns != new_nsproxy->ipc_ns)) {
Is the if (unshare_flags & CLONE_NEWIPC) check not enough?
Thanks,
Pavel
Quoted 19 lines · Manfred Spraul
Quoted 9 lines · Serge E. Hallyn
Quoted 9 lines
ipc/namespace.c +0
-0@@ -48,6 +48,16 @@ struct ipc_namespace *copy_ipcs(unsigned long flags, struct ipc_namespace *ns)if (!(flags &CLONE_NEWIPC ))return ns;Quoted 8 lines
ipc/namespace.c struct ipc_namespace *copy_ipcs(unsigned long flags, · line 53
Quoted 2 lines
ipc/namespace.c +1
-0+if (!(flags &CLONE_SYSVSEM )) {Wait, this should be opposite, right?
[snip]
Manfred, I'm trying to test this, but can't get an error without this patch. Do you have a testcase?
Quoted 7 lines
The patch is bogus, sorry that I didn't notice it immediately. The problem is
ipc/sem.c, functionfind_undo,lookup_undo:lookup_undodoesn't check the namespace pointer, thus a simple single threaded app can trigger the problem.Attached is a test app and a kernel patch that shows the problem. Run the test app immediately after boot (or within a new ipc namespace),
Thanks, Manfred. Basically what I was trying to do, except this appears to actually work :)
So now when I replace fork+unshare(CLONE_NEWNS|CLONE_NEWIPC) with clone(CLONE_NEWNS|CLONE_NEWIPC|CLONE_SYSVSEM) (moving the first semop to before the clone), I do in fact manage to get an oops with the mmotm I fetched this morning. So I maintain that you do also need to ensure at copy_namespaces() that if CLONE_NEWIPC is specified, then CLONE_SYSVSEM is *not* specified (since, this being clone, specifying it means that the semundo is shared with the parent).
thanks,
-serge
Quoted 196 lines
otherwise the ipc sequence counter will prevent the app from triggering the
problem: The undo structure that was created beforeunshare()[i.e. with 1 semaphore in it] will be used afterunshare()[i.e. semaphore 100 will be accessed].With kernel debugging (full slub debugging) enabled, I even got an oops when I tried to ipcrm the left over array after running undons, probably because the undo structure was freed at
exit_sem()within the new namespace, but still used in the outer namespace.--
Manfred/*
* Copyright (C) 1999,2001 by Manfred Spraul.
*
* Redistribution of this file is permitted under the terms of the GNU
* General Public License (GPL)
* $Header:/home/manfred/cvs-tree/manfred/ipcsem/undotest.c,v1.2 2003/06/28 15:19:43 manfred Exp $ */#ifdef __LINUX__ #define _GNU_SOURCE #include <sched.h> #include <wait.h> #define CLONE_NEWIPC 0x08000000 /* New ipcs */#endif
#include <sys/types.h>
#include <sys/ipc.h>
#include <sys/sem.h>
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <assert.h>
#include <signal.h>
#include <time.h>
#include <unistd.h>
#include <pthread.h>union semun { int val; struct semid_ds *buf; unsigned short int *array; struct seminfo* __buf; };int
getval(char * str, int id){ union semun arg; int res; res = semctl(id,0,GETVAL,arg); if(res==-1) { printf("GETVAL failed for %s.\n", str); exit(4); } printf(" %s: GETVAL now %d.\n",str, res); return res; }void setzero(int id)
{ union semun arg; int res; arg.val = 0; res = semctl(id,0,SETVAL,arg); if(res==-1) { printf("SETVAL failed, errno %d.\n", errno); exit(4); } printf(" SETVAL succeeded.\n"); }/* test1: verify that undo works at all. */
intmain(int argc,char** argv){ int res, id; printf(" ****************************************\n"); /* create array */ res = semget(IPC_PRIVATE, 1, 0700 | IPC_CREAT); printf(" got semaphore array %xh.\n",res); if(res == -1) { printf(" create failed.\n"); return 1; } id = res; setzero(id); res = getval("test1 init", id); if (res != 0) { printf("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n"); printf("Bad: got unexpected value.\n"); exit(99); } /* create sub-process */ res = fork(); if (res < 0) { printf("Fork failed (errno=%d). Aborting.\n", errno); res = semctl(id,1,IPC_RMID,NULL); exit(1); } fflush(stdout); if (!res) { struct sembuf sop[1]; int id2; sop[0].sem_num=0; sop[0].sem_op=1; sop[0].sem_flg=SEM_UNDO; res = semop(id,sop,1); if(res==-1) { printf("semop failed.\n"); exit(1); } res = getval("before unshare()", id); if (res != 1) { printf("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n"); printf("Bad: got unexpected value.\n"); exit(99); } fflush(stdout); errno = 0; res = unshare(CLONE_NEWNS|CLONE_NEWIPC); printf(" unshare returned %d, errno now %d (expected: 0,0).\n", res, errno); fflush(stdout); /* create array */ res = semget(IPC_PRIVATE, 101, 0700 | IPC_CREAT); printf(" got semaphore array %xh.\n",res); if(res == -1) { printf(" create failed.\n"); return 1; } id2 = res; printf("Now operating on id1=%x, id2=%x. (if both are equal, there will be a memory corruption)\n", id, id2); /* child: */ sop[0].sem_num=100; sop[0].sem_op=1; sop[0].sem_flg=SEM_UNDO; // This operation should trash kernel memory (if id1==id2) res = semop(id2,sop,1); if(res==-1) { printf("semop failed.\n"); exit(1); } exit(1); } else { int retval; retval = wait4(res, NULL, 0, NULL); if (retval != res) { printf("wait4 returned unexpeted value %d, errno now %d.\n", retval, errno); } res = getval("after exit", id); if (res != 0) { printf("XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX\n"); printf("Bad: got unexpected value.\n"); return 1; } } return 0; }ipc/sem.c +2
-0@@ -1081,6 +1082,7 @@ static struct sem_undo *find_undo(struct ipc_namespace *ns, int semid) new->id_next = sma->undo; sma->undo = new;sem_unlock (sma); +printk (KERN_ERR "find_undo(%p, %x) new undop %p (for %d sems).\n" , ns, semid, new, nsems); un = new;spin_unlock (&ulp->lock); out:@@ -1153,6 +1155,7 @@ retry_undos: error =PTR_ERR (sma); goto out_free; } +printk (KERN_ERR "find_undo(%p, %x) == %p, using %ld sems.\n" , ns, semid, un, sma->sem_nsems);/* * semid identifiers are not unique - find_undo may have