[PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Davidlohr Bueso]

Upon every shm_lock call, we BUG_ON if an error was returned,
indicating racing either in idr or in RMID. Move this logic
into the locking.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/shm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 6d76707..3152dea 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
 {
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
-	if (IS_ERR(ipcp))
+	if (IS_ERR(ipcp)) {
+		/*
+		 * We raced in the idr lookup or with RMID,
+		 * either way, the ID is busted.
+		 */
+		BUG_ON(1);
 		return (struct shmid_kernel *)ipcp;
+	}
 
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
@@ -191,7 +197,6 @@ static void shm_open(struct vm_area_struct *vma)
 	struct shmid_kernel *shp;
 
 	shp = shm_lock(sfd->ns, sfd->id);
-	BUG_ON(IS_ERR(shp));
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
@@ -258,7 +263,6 @@ static void shm_close(struct vm_area_struct *vma)
 	down_write(&shm_ids(ns).rwsem);
 	/* remove from the list of attaches of the shm segment */
 	shp = shm_lock(ns, sfd->id);
-	BUG_ON(IS_ERR(shp));
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_dtim = get_seconds();
 	shp->shm_nattch--;
@@ -1191,7 +1195,6 @@ out_fput:
 out_nattch:
 	down_write(&shm_ids(ns).rwsem);
 	shp = shm_lock(ns, shmid);
-	BUG_ON(IS_ERR(shp));
 	shp->shm_nattch--;
 	if (shm_may_destroy(ns, shp))
 		shm_destroy(ns, shp);
-- 
2.1.4

[PATCH 2/2] ipc,msg: provide barrier pairings for lockless receive

[Davidlohr Bueso]

We currently use a full barrier on the sender side to
to avoid receiver tasks disappearing on us while still
performing on the sender side wakeup. We lack however,
the proper CPU-CPU interactions pairing on the receiver
side which busy-waits for the message. Similarly, we do
not need a full smp_mb, and can relax the semantics for
the writer and reader sides of the message. This is safe
as we are only ordering loads and stores to r_msg. And in
both smp_wmb and smp_rmb, there are no stores after the
calls _anyway_.

This obviously applies for pipelined_send and expunge_all,
for EIRDM when destroying a queue.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/msg.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 2b6fdbb..ac5116e 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -196,7 +196,7 @@ static void expunge_all(struct msg_queue *msq, int res)
 		 * or dealing with -EAGAIN cases. See lockless receive part 1
 		 * and 2 in do_msgrcv().
 		 */
-		smp_mb();
+		smp_wmb();
 		msr->r_msg = ERR_PTR(res);
 	}
 }
@@ -580,7 +580,7 @@ static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg)
 				/* initialize pipelined send ordering */
 				msr->r_msg = NULL;
 				wake_up_process(msr->r_tsk);
-				smp_mb(); /* see barrier comment below */
+				smp_wmb(); /* see barrier comment below */
 				msr->r_msg = ERR_PTR(-E2BIG);
 			} else {
 				msr->r_msg = NULL;
@@ -589,11 +589,12 @@ static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg)
 				wake_up_process(msr->r_tsk);
 				/*
 				 * Ensure that the wakeup is visible before
-				 * setting r_msg, as the receiving end depends
-				 * on it. See lockless receive part 1 and 2 in
-				 * do_msgrcv().
+				 * setting r_msg, as the receiving can otherwise
+				 * exit - once r_msg is set, the receiver can
+				 * continue. See lockless receive part 1 and 2
+				 * in do_msgrcv().
 				 */
-				smp_mb();
+				smp_wmb();
 				msr->r_msg = msg;
 
 				return 1;
@@ -934,10 +935,22 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
 		 * wake_up_process(). There is a race with exit(), see
 		 * ipc/mqueue.c for the details.
 		 */
-		msg = (struct msg_msg *)msr_d.r_msg;
-		while (msg == NULL) {
-			cpu_relax();
+		for (;;) {
+			/*
+			 * Pairs with writer barrier in pipelined_send
+			 * or expunge_all
+			 */
+			smp_rmb();
 			msg = (struct msg_msg *)msr_d.r_msg;
+			if (msg)
+				break;
+
+			/*
+			 * The cpu_relax() call is a compiler barrier
+			 * which forces everything in this loop to be
+			 * re-loaded.
+			 */
+			cpu_relax();
 		}
 
 		/* Lockless receive, part 3:
-- 
2.1.4

Re: [PATCH 2/2] ipc,msg: provide barrier pairings for lockless receive

[Manfred Spraul]

On 05/30/2015 02:03 AM, Davidlohr Bueso wrote:
> We currently use a full barrier on the sender side to
> to avoid receiver tasks disappearing on us while still
> performing on the sender side wakeup. We lack however,
> the proper CPU-CPU interactions pairing on the receiver
> side which busy-waits for the message. Similarly, we do
> not need a full smp_mb, and can relax the semantics for
> the writer and reader sides of the message. This is safe
> as we are only ordering loads and stores to r_msg. And in
> both smp_wmb and smp_rmb, there are no stores after the
> calls _anyway_.
I like the idea, the pairing in ipc is not good.
Another one is still open in sem.

Perhaps we should formalize it a bit more, so that it is easy to find 
which barrier pair belongs together.
It is only an idea, but right now there are too many bugs.

> This obviously applies for pipelined_send and expunge_all,
> for EIRDM when destroying a queue.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>   ipc/msg.c | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 2b6fdbb..ac5116e 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -196,7 +196,7 @@ static void expunge_all(struct msg_queue *msq, int res)
>   		 * or dealing with -EAGAIN cases. See lockless receive part 1
>   		 * and 2 in do_msgrcv().
>   		 */
> -		smp_mb();
> +		smp_wmb();
Idea for improvement: Add here /* smp_barrier_pair: ipc_msg_01 */

>   		msr->r_msg = ERR_PTR(res);
>   	}
>   }
> @@ -580,7 +580,7 @@ static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg)
>   				/* initialize pipelined send ordering */
>   				msr->r_msg = NULL;
>   				wake_up_process(msr->r_tsk);
> -				smp_mb(); /* see barrier comment below */
> +				smp_wmb(); /* see barrier comment below */
>   				msr->r_msg = ERR_PTR(-E2BIG);
>   			} else {
>   				msr->r_msg = NULL;
> @@ -589,11 +589,12 @@ static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg)
>   				wake_up_process(msr->r_tsk);
>   				/*
>   				 * Ensure that the wakeup is visible before
> -				 * setting r_msg, as the receiving end depends
> -				 * on it. See lockless receive part 1 and 2 in
> -				 * do_msgrcv().
> +				 * setting r_msg, as the receiving can otherwise
> +				 * exit - once r_msg is set, the receiver can
> +				 * continue. See lockless receive part 1 and 2
> +				 * in do_msgrcv().
>   				 */
> -				smp_mb();
> +				smp_wmb();
Idea for improvement: Add here /* smp_barrier_pair: ipc_msg_02 */
>   				msr->r_msg = msg;
>   
>   				return 1;
> @@ -934,10 +935,22 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
>   		 * wake_up_process(). There is a race with exit(), see
>   		 * ipc/mqueue.c for the details.
>   		 */
> -		msg = (struct msg_msg *)msr_d.r_msg;
> -		while (msg == NULL) {
> -			cpu_relax();
> +		for (;;) {
> +			/*
> +			 * Pairs with writer barrier in pipelined_send
> +			 * or expunge_all
> +			 */
> +			smp_rmb();
And here again /* smp_barrier_pair: for ipc_msg_01 and ipc_msg_02 */

--
     Manfred

Re: [PATCH 2/2] ipc,msg: provide barrier pairings for lockless receive

[Davidlohr Bueso]

On Thu, 2015-06-04 at 19:57 +0200, Manfred Spraul wrote:
> On 05/30/2015 02:03 AM, Davidlohr Bueso wrote:
> > We currently use a full barrier on the sender side to
> > to avoid receiver tasks disappearing on us while still
> > performing on the sender side wakeup. We lack however,
> > the proper CPU-CPU interactions pairing on the receiver
> > side which busy-waits for the message. Similarly, we do
> > not need a full smp_mb, and can relax the semantics for
> > the writer and reader sides of the message. This is safe
> > as we are only ordering loads and stores to r_msg. And in
> > both smp_wmb and smp_rmb, there are no stores after the
> > calls _anyway_.
> I like the idea, the pairing in ipc is not good.
> Another one is still open in sem.

Hmm for sems are you referring to spinning on ->status in
get_queue_result() while another task is performing a wakeup in between
wake_up_sem_queue_prepare() and wake_up_sem_queue_do()?

> 
> Perhaps we should formalize it a bit more, so that it is easy to find 
> which barrier pair belongs together.
> It is only an idea, but right now there are too many bugs.

Good point, however, what do you think of the below instead? Makes it
crystal clear, imho.

Thanks,
Davidlohr

diff --git a/ipc/msg.c b/ipc/msg.c
index 2b6fdbb..ce7bf50 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -196,7 +196,7 @@ static void expunge_all(struct msg_queue *msq, int res)
 		 * or dealing with -EAGAIN cases. See lockless receive part 1
 		 * and 2 in do_msgrcv().
 		 */
-		smp_mb();
+		smp_wmb(); /* barrier (B) */
 		msr->r_msg = ERR_PTR(res);
 	}
 }
@@ -580,7 +580,8 @@ static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg)
 				/* initialize pipelined send ordering */
 				msr->r_msg = NULL;
 				wake_up_process(msr->r_tsk);
-				smp_mb(); /* see barrier comment below */
+				/* barrier (B) see barrier comment below */
+				smp_wmb();
 				msr->r_msg = ERR_PTR(-E2BIG);
 			} else {
 				msr->r_msg = NULL;
@@ -589,11 +590,12 @@ static inline int pipelined_send(struct msg_queue *msq, struct msg_msg *msg)
 				wake_up_process(msr->r_tsk);
 				/*
 				 * Ensure that the wakeup is visible before
-				 * setting r_msg, as the receiving end depends
-				 * on it. See lockless receive part 1 and 2 in
-				 * do_msgrcv().
+				 * setting r_msg, as the receiving can otherwise
+				 * exit - once r_msg is set, the receiver can
+				 * continue. See lockless receive part 1 and 2
+				 * in do_msgrcv(). Barrier (B).
 				 */
-				smp_mb();
+				smp_wmb();
 				msr->r_msg = msg;
 
 				return 1;
@@ -932,12 +934,38 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
 		/* Lockless receive, part 2:
 		 * Wait until pipelined_send or expunge_all are outside of
 		 * wake_up_process(). There is a race with exit(), see
-		 * ipc/mqueue.c for the details.
+		 * ipc/mqueue.c for the details. The correct serialization
+		 * ensures that a receiver cannot continue without the wakeup
+		 * being visibible _before_ setting r_msg:
+		 *
+		 * CPU 0                             CPU 1
+		 * <loop receiver>
+		 *   smp_rmb(); (A) <-- pair -.      <waker thread>
+		 *   <load ->r_msg>           |        msr->r_msg = NULL;
+		 *                            |        wakeup_process()
+		 * <continue>                 `------> smp_wmb(); (B)
+		 *                                     msr->r_msg = msg;
+		 *
+		 * Where (A) orders the message value read and where (B) orders
+		 * the write to the futex -- done in both pipelined_send and
+		 * expunge_all.		 *
 		 */
-		msg = (struct msg_msg *)msr_d.r_msg;
-		while (msg == NULL) {
-			cpu_relax();
+		for (;;) {
+			/*
+			 * Pairs with writer barrier in pipelined_send
+			 * or expunge_all.
+			 */
+			smp_rmb(); /* barrier (A) */
 			msg = (struct msg_msg *)msr_d.r_msg;
+			if (msg)
+				break;
+
+			/*
+			 * The cpu_relax() call is a compiler barrier
+			 * which forces everything in this loop to be
+			 * re-loaded.
+			 */
+			cpu_relax();
 		}
 
 		/* Lockless receive, part 3:

Re: [PATCH 2/2] ipc,msg: provide barrier pairings for lockless receive

[Davidlohr Bueso]

On Thu, 2015-06-04 at 11:41 -0700, Davidlohr Bueso wrote:
> On Thu, 2015-06-04 at 19:57 +0200, Manfred Spraul wrote:
> > On 05/30/2015 02:03 AM, Davidlohr Bueso wrote:
> > > We currently use a full barrier on the sender side to
> > > to avoid receiver tasks disappearing on us while still
> > > performing on the sender side wakeup. We lack however,
> > > the proper CPU-CPU interactions pairing on the receiver
> > > side which busy-waits for the message. Similarly, we do
> > > not need a full smp_mb, and can relax the semantics for
> > > the writer and reader sides of the message. This is safe
> > > as we are only ordering loads and stores to r_msg. And in
> > > both smp_wmb and smp_rmb, there are no stores after the
> > > calls _anyway_.
> > I like the idea, the pairing in ipc is not good.
> > Another one is still open in sem.
> 
> Hmm for sems are you referring to spinning on ->status in
> get_queue_result() while another task is performing a wakeup in between
> wake_up_sem_queue_prepare() and wake_up_sem_queue_do()?
> 
> > 
> > Perhaps we should formalize it a bit more, so that it is easy to find 
> > which barrier pair belongs together.
> > It is only an idea, but right now there are too many bugs.
> 
> Good point, however, what do you think of the below instead? Makes it
> crystal clear, imho.

We had to do some formalizing in futex too.

> +		 * Where (A) orders the message value read and where (B) orders
> +		 * the write to the futex -- done in both pipelined_send and
                                      ^^ this should be r_msg

Re: [PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Andrew Morton]

On Fri, 29 May 2015 17:03:05 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:

> Upon every shm_lock call, we BUG_ON if an error was returned,
> indicating racing either in idr or in RMID. Move this logic
> into the locking.
> 
> ...
>
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
>  {
>  	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
>  
> -	if (IS_ERR(ipcp))
> +	if (IS_ERR(ipcp)) {
> +		/*
> +		 * We raced in the idr lookup or with RMID,
> +		 * either way, the ID is busted.
> +		 */
> +		BUG_ON(1);
>  		return (struct shmid_kernel *)ipcp;
> +	}
>  

That's a bit odd.  Why not

--- a/ipc/shm.c~ipcshm-move-bug_on-check-into-shm_lock-fix-2
+++ a/ipc/shm.c
@@ -160,7 +160,7 @@ static inline struct shmid_kernel *shm_l
 		 * We raced in the idr lookup or with RMID,
 		 * either way, the ID is busted.
 		 */
-		BUG_ON(1);
+		BUG();
 		return (struct shmid_kernel *)ipcp;
 	}
 

and/or

--- a/ipc/shm.c~ipcshm-move-bug_on-check-into-shm_lock-fix
+++ a/ipc/shm.c
@@ -155,14 +155,11 @@ static inline struct shmid_kernel *shm_l
 {
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
-	if (IS_ERR(ipcp)) {
-		/*
-		 * We raced in the idr lookup or with RMID,
-		 * either way, the ID is busted.
-		 */
-		BUG();
-		return (struct shmid_kernel *)ipcp;
-	}
+	/*
+	 * We raced in the idr lookup or with RMID.  Either way, the ID is
+	 * busted.
+	 */
+	BUG_ON(IS_ERR(ipcp));
 
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
_

Re: [PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Davidlohr Bueso]

On Mon, 2015-06-01 at 15:52 -0700, Andrew Morton wrote:
> On Fri, 29 May 2015 17:03:05 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > Upon every shm_lock call, we BUG_ON if an error was returned,
> > indicating racing either in idr or in RMID. Move this logic
> > into the locking.
> > 
> > ...
> >
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
> >  {
> >  	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
> >  
> > -	if (IS_ERR(ipcp))
> > +	if (IS_ERR(ipcp)) {
> > +		/*
> > +		 * We raced in the idr lookup or with RMID,
> > +		 * either way, the ID is busted.
> > +		 */
> > +		BUG_ON(1);
> >  		return (struct shmid_kernel *)ipcp;
> > +	}
> >  

Sure, this is fine.

Thanks,
Davidlohr

Re: [PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Davidlohr Bueso]

On Mon, 2015-06-01 at 15:52 -0700, Andrew Morton wrote:
> On Fri, 29 May 2015 17:03:05 -0700 Davidlohr Bueso <dave@stgolabs.net> wrote:
> 
> > Upon every shm_lock call, we BUG_ON if an error was returned,
> > indicating racing either in idr or in RMID. Move this logic
> > into the locking.
> > 
> > ...
> >
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
> >  {
> >  	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
> >  
> > -	if (IS_ERR(ipcp))
> > +	if (IS_ERR(ipcp)) {
> > +		/*
> > +		 * We raced in the idr lookup or with RMID,
> > +		 * either way, the ID is busted.
> > +		 */
> > +		BUG_ON(1);
> >  		return (struct shmid_kernel *)ipcp;
> > +	}
> >  
> 
> That's a bit odd.  Why not
> 
> --- a/ipc/shm.c~ipcshm-move-bug_on-check-into-shm_lock-fix-2
> +++ a/ipc/shm.c
> @@ -160,7 +160,7 @@ static inline struct shmid_kernel *shm_l
>  		 * We raced in the idr lookup or with RMID,
>  		 * either way, the ID is busted.
>  		 */
> -		BUG_ON(1);
> +		BUG();
>  		return (struct shmid_kernel *)ipcp;
>  	}

Sure, this is fine.

Thanks,
Davidlohr

Re: [PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Manfred Spraul]

Hi Davidlohr,

On 05/30/2015 02:03 AM, Davidlohr Bueso wrote:
> Upon every shm_lock call, we BUG_ON if an error was returned,
> indicating racing either in idr or in RMID. Move this logic
> into the locking.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>   ipc/shm.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 6d76707..3152dea 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
>   {
>   	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
>   
> -	if (IS_ERR(ipcp))
> +	if (IS_ERR(ipcp)) {
> +		/*
> +		 * We raced in the idr lookup or with RMID,
> +		 * either way, the ID is busted.
The comment is wrong:
"We raced in the idr lookup or with shm_destroy()"

shm is not like msg or sem:
RMID merely marks a shmid as deletable (SHM_DEST), delete (i.e.: 
shm_rmid(), then ipc_rmid()) happens only after the last attached 
segment is detatched.

And: (unrelated to the patch)
For do_shmat(), I'm not 100% sure that the BUG can't be triggered.

--
     Manfred

Re: [PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Davidlohr Bueso]

On Thu, 2015-06-04 at 20:35 +0200, Manfred Spraul wrote:
> Hi Davidlohr,
> 
> On 05/30/2015 02:03 AM, Davidlohr Bueso wrote:
> > Upon every shm_lock call, we BUG_ON if an error was returned,
> > indicating racing either in idr or in RMID. Move this logic
> > into the locking.
> >
> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> > ---
> >   ipc/shm.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 6d76707..3152dea 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
> >   {
> >   	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
> >   
> > -	if (IS_ERR(ipcp))
> > +	if (IS_ERR(ipcp)) {
> > +		/*
> > +		 * We raced in the idr lookup or with RMID,
> > +		 * either way, the ID is busted.
> The comment is wrong:
> "We raced in the idr lookup or with shm_destroy()"

Yes, you are absolutely right. Thanks for the review.

Re: [PATCH 1/2] ipc,shm: move BUG_ON check into shm_lock

[Davidlohr Bueso]

On Thu, 2015-06-04 at 20:35 +0200, Manfred Spraul wrote:
> Hi Davidlohr,
> 
> On 05/30/2015 02:03 AM, Davidlohr Bueso wrote:
> > Upon every shm_lock call, we BUG_ON if an error was returned,
> > indicating racing either in idr or in RMID. Move this logic
> > into the locking.
> >
> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> > ---
> >   ipc/shm.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 6d76707..3152dea 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -155,8 +155,14 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
> >   {
> >   	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
> >   
> > -	if (IS_ERR(ipcp))
> > +	if (IS_ERR(ipcp)) {
> > +		/*
> > +		 * We raced in the idr lookup or with RMID,
> > +		 * either way, the ID is busted.
> The comment is wrong:
> "We raced in the idr lookup or with shm_destroy()"
> 
> shm is not like msg or sem:
> RMID merely marks a shmid as deletable (SHM_DEST), delete (i.e.: 
> shm_rmid(), then ipc_rmid()) happens only after the last attached 
> segment is detatched.
> 
> And: (unrelated to the patch)
> For do_shmat(), I'm not 100% sure that the BUG can't be triggered.

Could you be referring to the fact that we don't mark the segment
->deleted when doing RMID (do_shm_rmid)? This would certainly cause
checks to ipc_valid_object() to completely screw up. I've actually been
chasing that BUG() in shm_open and shm_close during forking and exiting
(unmapping) respectively.

*Completely untested*

Thanks,
Davidlohr

diff --git a/ipc/shm.c b/ipc/shm.c
index 3323c49..4e78cd2 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -95,6 +95,14 @@ static void do_shm_rmid(struct ipc_namespace *ns, struct kern_ipc_perm *ipcp)
 		shp->shm_perm.mode |= SHM_DEST;
 		/* Do not find it any more */
 		shp->shm_perm.key = IPC_PRIVATE;
+		/*
+		 * Mark the object deleted. As opposed to
+		 * sems or msg queues, shm only marks the
+		 * shmid deletable upon RMID, and delays
+		 * the freeing until there are no more
+		 * active users of the segment.
+		 */
+		shp->shm_perm.deleted = true;
 		shm_unlock(shp);
 	} else
 		shm_destroy(ns, shp);