Messages in this thread Patch in this message | | | Date | Sun, 06 Apr 2008 17:11:32 +0200 | From | Manfred Spraul <> | Subject | Re: [RFC, PATCH] fix SEM_UNDO with namespaces |
| |
Serge E. Hallyn wrote: >> diff --git a/ipc/namespace.c b/ipc/namespace.c >> index 9171d94..9044505 100644 >> --- a/ipc/namespace.c >> +++ b/ipc/namespace.c >> @@ -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? > > [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; } diff --git a/ipc/sem.c b/ipc/sem.c index 0b45a4d..5c882f9 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -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 | |