Re: + shm-fix-a-race-between-shm_exit-and-shm_init.patch added to -mm tree

Re: + shm-fix-a-race-between-shm_exit-and-shm_init.patch added to -mm tree

[Oleg Nesterov]

> From: Vasiliy Kulikov <segoon@openwall.com>
>
> On thread exit shm_exit_ns() is called, it uses shm_ids(ns).rw_mutex.  It
> is initialized in shm_init(), but it is not called yet at the moment of
> kernel threads exit.  Some kernel threads are created in
> do_pre_smp_initcalls(), and shm_init() is called in do_initcalls().
>
> Static initialization of shm_ids(init_ipc_ns).rw_mutex fixes the race.

Yes, it is safe to call down_right() now.

But the code does

	down_write(rw_mutex);
	if (.in_use)
		idr_for_each(.ipcs_idr);

and thus it relies on the static initializer anyway. it is not safe
to do idr_for_each() before idr_init() in theory.

And since we rely on .in_use == 0, why we can't move this check
outside of down_write/up_right to a) optimize the code and b)
fix the problem?

Oleg.

Re: + shm-fix-a-race-between-shm_exit-and-shm_init.patch added to -mm tree

[Vasiliy Kulikov]

On Wed, Aug 03, 2011 at 16:04 +0200, Oleg Nesterov wrote:
> > From: Vasiliy Kulikov <segoon@openwall.com>
> >
> > On thread exit shm_exit_ns() is called, it uses shm_ids(ns).rw_mutex.  It
> > is initialized in shm_init(), but it is not called yet at the moment of
> > kernel threads exit.  Some kernel threads are created in
> > do_pre_smp_initcalls(), and shm_init() is called in do_initcalls().
> >
> > Static initialization of shm_ids(init_ipc_ns).rw_mutex fixes the race.
> 
> Yes, it is safe to call down_right() now.
> 
> But the code does
> 
> 	down_write(rw_mutex);
> 	if (.in_use)
> 		idr_for_each(.ipcs_idr);
> 
> and thus it relies on the static initializer anyway. it is not safe
> to do idr_for_each() before idr_init() in theory.
> 
> And since we rely on .in_use == 0, why we can't move this check
> outside of down_write/up_right to a) optimize the code and b)
> fix the problem?

Agreed.  But I second Linus that partial initialization only hides the
real problem.  And some initcall chain movement is still needed.

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

[PATCH] shm: fix wrong tests

[Vasiliy Kulikov]

The patch 4c677e2eefdba9c5bfc4474e2e9 introduced a copy-paste bug.
Due to the bug cycle optimizations were disabled.

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 ipc/shm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index bf46636..4e3c883 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -294,7 +294,7 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
 void shm_destroy_orphaned(struct ipc_namespace *ns)
 {
 	down_write(&shm_ids(ns).rw_mutex);
-	if (&shm_ids(ns).in_use)
+	if (shm_ids(ns).in_use)
 		idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
 	up_write(&shm_ids(ns).rw_mutex);
 }
@@ -306,7 +306,7 @@ void exit_shm(struct task_struct *task)
 
 	/* Destroy all already created segments, but not mapped yet */
 	down_write(&shm_ids(ns).rw_mutex);
-	if (&shm_ids(ns).in_use)
+	if (shm_ids(ns).in_use)
 		idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_current, ns);
 	up_write(&shm_ids(ns).rw_mutex);
 }
-- 
1.7.0.4

[PATCH] shm: optimize exit_shm()

[Vasiliy Kulikov]

We may check .in_use == 0 without holding the rw_mutex as .in_use is int
and reads of ints are atomic.  As .in_use may be changed to zero while current
process was sleeping in down_write(), we should check .in_use once again after
down_write().

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 ipc/shm.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 4e3c883..855ddc0 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -304,6 +304,9 @@ void exit_shm(struct task_struct *task)
 {
 	struct ipc_namespace *ns = task->nsproxy->ipc_ns;
 
+	if (shm_ids(ns).in_use == 0)
+		return;
+
 	/* Destroy all already created segments, but not mapped yet */
 	down_write(&shm_ids(ns).rw_mutex);
 	if (shm_ids(ns).in_use)
-- 
1.7.0.4

Re: [PATCH] shm: optimize exit_shm()

[Manuel Lauss]

Hello,

On Wed, Aug 3, 2011 at 8:28 PM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> We may check .in_use == 0 without holding the rw_mutex as .in_use is int
> and reads of ints are atomic.  As .in_use may be changed to zero while current
> process was sleeping in down_write(), we should check .in_use once again after
> down_write().
>
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> ---
>  ipc/shm.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 4e3c883..855ddc0 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -304,6 +304,9 @@ void exit_shm(struct task_struct *task)
>  {
>        struct ipc_namespace *ns = task->nsproxy->ipc_ns;
>
> +       if (shm_ids(ns).in_use == 0)
> +               return;
> +
>        /* Destroy all already created segments, but not mapped yet */
>        down_write(&shm_ids(ns).rw_mutex);
>        if (shm_ids(ns).in_use)

This check here is now unnecessary, yes?

And this also fixes the oops.

Manuel

Re: [PATCH] shm: optimize exit_shm()

[Vasiliy Kulikov]

Hi Manuel,

On Wed, Aug 03, 2011 at 21:08 +0200, Manuel Lauss wrote:
> On Wed, Aug 3, 2011 at 8:28 PM, Vasiliy Kulikov <segoon@openwall.com> wrote:
> > We may check .in_use == 0 without holding the rw_mutex as .in_use is int
> > and reads of ints are atomic.  As .in_use may be changed to zero while current
> > process was sleeping in down_write(), we should check .in_use once again after
> > down_write().
[...]
> > +       if (shm_ids(ns).in_use == 0)
> > +               return;
> > +
> >        /* Destroy all already created segments, but not mapped yet */
> >        down_write(&shm_ids(ns).rw_mutex);
> >        if (shm_ids(ns).in_use)
> 
> This check here is now unnecessary, yes?

No, as I said in the comment above, other task may be holding the mutex and
deleting the last shm segment.  So, current task will see in_use == 1
before down_write(), but == 0 after it.

> And this also fixes the oops.

Yes, but it only hides the real problem - tasks' dependency on initialized
init_*_ns.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] shm: optimize exit_shm()

[Oleg Nesterov]

On 08/03, Vasiliy Kulikov wrote:
>
> On Wed, Aug 03, 2011 at 21:08 +0200, Manuel Lauss wrote:
> > > +
> > >        /* Destroy all already created segments, but not mapped yet */
> > >        down_write(&shm_ids(ns).rw_mutex);
> > >        if (shm_ids(ns).in_use)
> >
> > This check here is now unnecessary, yes?
>
> No, as I said in the comment above, other task may be holding the mutex and
> deleting the last shm segment.  So, current task will see in_use == 1
> before down_write(), but == 0 after it.

And? Why we can not do idr_for_each() in this (unikely) case?

> > And this also fixes the oops.
>
> Yes, but it only hides the real problem - tasks' dependency on initialized
> init_*_ns.

This is true, but your patch has the same dependency, but pretends
it doesn't ;) and it complicates the code.

Oleg.

Re: [PATCH] shm: optimize exit_shm()

[Vasiliy Kulikov]

On Wed, Aug 03, 2011 at 21:29 +0200, Oleg Nesterov wrote:
> On 08/03, Vasiliy Kulikov wrote:
> >
> > On Wed, Aug 03, 2011 at 21:08 +0200, Manuel Lauss wrote:
> > > > +
> > > >        /* Destroy all already created segments, but not mapped yet */
> > > >        down_write(&shm_ids(ns).rw_mutex);
> > > >        if (shm_ids(ns).in_use)
> > >
> > > This check here is now unnecessary, yes?
> >
> > No, as I said in the comment above, other task may be holding the mutex and
> > deleting the last shm segment.  So, current task will see in_use == 1
> > before down_write(), but == 0 after it.
> 
> And? Why we can not do idr_for_each() in this (unikely) case?

Because it's pointless.  idr_for_each() would not find any used segment.

> > > And this also fixes the oops.
> >
> > Yes, but it only hides the real problem - tasks' dependency on initialized
> > init_*_ns.
> 
> This is true, but your patch has the same dependency, but pretends
> it doesn't ;) and it complicates the code.

I didn't say that .in_use check fixes the oops.  The description says
nothing about it.  It does an optimization, which is said in the
description.

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] shm: optimize exit_shm()

[Oleg Nesterov]

On 08/03, Vasiliy Kulikov wrote:
>
> On Wed, Aug 03, 2011 at 21:29 +0200, Oleg Nesterov wrote:
> > On 08/03, Vasiliy Kulikov wrote:
> > >
> > > On Wed, Aug 03, 2011 at 21:08 +0200, Manuel Lauss wrote:
> > > > > +
> > > > >        /* Destroy all already created segments, but not mapped yet */
> > > > >        down_write(&shm_ids(ns).rw_mutex);
> > > > >        if (shm_ids(ns).in_use)
> > > >
> > > > This check here is now unnecessary, yes?
> > >
> > > No, as I said in the comment above, other task may be holding the mutex and
> > > deleting the last shm segment.  So, current task will see in_use == 1
> > > before down_write(), but == 0 after it.
> >
> > And? Why we can not do idr_for_each() in this (unikely) case?
>
> Because it's pointless.  idr_for_each() would not find any used segment.

This is clear. But it seems that me + Manuel were equally confused
by the changelog.

> > > > And this also fixes the oops.
> > >
> > > Yes, but it only hides the real problem - tasks' dependency on initialized
> > > init_*_ns.
> >
> > This is true, but your patch has the same dependency, but pretends
> > it doesn't ;) and it complicates the code.
>
> I didn't say that .in_use check fixes the oops.

I meant your shm-fix-a-race-between-shm_exit-and-shm_init.patch
which should be dropped imho ;)

Oleg.

Re: [PATCH] shm: optimize exit_shm()

[Oleg Nesterov]

On 08/03, Vasiliy Kulikov wrote:
>
> As .in_use may be changed to zero while current
> process was sleeping in down_write(),

Yes,

> we should check .in_use once again after
> down_write().

Why?

It doesn't hurt to recheck and I think the patch is correct.
But the changelog looks confusing.

Oleg.

Re: [PATCH] shm: optimize exit_shm()

[Vasiliy Kulikov]

On Wed, Aug 03, 2011 at 21:21 +0200, Oleg Nesterov wrote:
> > we should check .in_use once again after
> > down_write().
> 
> Why?

https://lkml.org/lkml/2011/8/3/277

"No, as I said in the comment above, other task may be holding the mutex and
deleting the last shm segment.  So, current task will see in_use == 1
before down_write(), but == 0 after it."

"Should" == additional check might speed the things, so it worth checking.

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] shm: optimize exit_shm()

[Oleg Nesterov]

On 08/03, Vasiliy Kulikov wrote:
>
> On Wed, Aug 03, 2011 at 21:21 +0200, Oleg Nesterov wrote:
> > > we should check .in_use once again after
> > > down_write().
> >
> > Why?
>
> https://lkml.org/lkml/2011/8/3/277
>
> "No, as I said in the comment above, other task may be holding the mutex and
> deleting the last shm segment.

This is obvious,

> "Should" == additional check might speed the things, so it worth checking.

and this is not.

I was confused, the changelog looks as if we _have to_ recheck
or something bad can happen.

But as I said, the patch looks correct anyway. I am not
sure the 2nd optimization really makes sense (this is very
unlikely case) but it doesn't hurt.

Oleg.

Re: + shm-fix-a-race-between-shm_exit-and-shm_init.patch added to -mm tree

[Oleg Nesterov]

On 08/03, Vasiliy Kulikov wrote:
>
> On Wed, Aug 03, 2011 at 16:04 +0200, Oleg Nesterov wrote:
> >
> > And since we rely on .in_use == 0, why we can't move this check
> > outside of down_write/up_right to a) optimize the code and b)
> > fix the problem?
>
> Agreed.  But I second Linus that partial initialization only hides the
> real problem.  And some initcall chain movement is still needed.

I agree. But, until then, we can make a much more simple fix.
Especially because this patch (imho) mostly hides the problem too.

Oleg.

+ shm-fix-a-race-between-shm_exit-and-shm_init.patch added to -mm tree

[akpm]

The patch titled
     shm: fix a race between shm_exit() and shm_init()
has been added to the -mm tree.  Its filename is
     shm-fix-a-race-between-shm_exit-and-shm_init.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: shm: fix a race between shm_exit() and shm_init()
From: Vasiliy Kulikov <segoon@openwall.com>

On thread exit shm_exit_ns() is called, it uses shm_ids(ns).rw_mutex.  It
is initialized in shm_init(), but it is not called yet at the moment of
kernel threads exit.  Some kernel threads are created in
do_pre_smp_initcalls(), and shm_init() is called in do_initcalls().

Static initialization of shm_ids(init_ipc_ns).rw_mutex fixes the race.

It fixes a kernel oops:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
[<c0320090>] (__down_write_nested+0x88/0xe0) from [<c015da08>] (exit_shm+0x28/0x48)
[<c015da08>] (exit_shm+0x28/0x48) from [<c002e550>] (do_exit+0x59c/0x750)
[<c002e550>] (do_exit+0x59c/0x750) from [<c003eaac>] (____call_usermodehelper+0x13c/0x154)
[<c003eaac>] (____call_usermodehelper+0x13c/0x154) from [<c000f630>] (kernel_thread_exit+0x0/0x8)
Code: 1afffffa e597c00c e58d0000 e587d00c (e58cd000)

Reported-by: Manuel Lauss <manuel.lauss@googlemail.com>
Reported-by: Richard Weinberger <richard@nod.at>
Reported-by: Marc Zyngier <maz@misterjones.org>
Tested-by: Manuel Lauss <manuel.lauss@googlemail.com>
Tested-by: Richard Weinberger <richard@nod.at>
Tested-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 ipc/msgutil.c |    6 ++++++
 ipc/shm.c     |   11 ++++++++++-
 ipc/util.c    |   30 +++++++++++++++++-------------
 ipc/util.h    |    1 +
 4 files changed, 34 insertions(+), 14 deletions(-)

diff -puN ipc/msgutil.c~shm-fix-a-race-between-shm_exit-and-shm_init ipc/msgutil.c
--- a/ipc/msgutil.c~shm-fix-a-race-between-shm_exit-and-shm_init
+++ a/ipc/msgutil.c
@@ -20,6 +20,9 @@
 
 DEFINE_SPINLOCK(mq_lock);
 
+#define INIT_IPC_SHM_IDS(name) \
+	{ .rw_mutex = __RWSEM_INITIALIZER(name.rw_mutex), }
+
 /*
  * The next 2 defines are here bc this is the only file
  * compiled when either CONFIG_SYSVIPC and CONFIG_POSIX_MQUEUE
@@ -27,6 +30,9 @@ DEFINE_SPINLOCK(mq_lock);
  */
 struct ipc_namespace init_ipc_ns = {
 	.count		= ATOMIC_INIT(1),
+	.ids	= {
+		[IPC_SHM_IDS] = INIT_IPC_SHM_IDS(init_ipc_ns.ids[IPC_SHM_IDS]),
+	},
 #ifdef CONFIG_POSIX_MQUEUE
 	.mq_queues_max   = DFLT_QUEUESMAX,
 	.mq_msg_max      = DFLT_MSGMAX,
diff -puN ipc/shm.c~shm-fix-a-race-between-shm_exit-and-shm_init ipc/shm.c
--- a/ipc/shm.c~shm-fix-a-race-between-shm_exit-and-shm_init
+++ a/ipc/shm.c
@@ -76,7 +76,16 @@ void shm_init_ns(struct ipc_namespace *n
 	ns->shm_ctlmni = SHMMNI;
 	ns->shm_rmid_forced = 0;
 	ns->shm_tot = 0;
-	ipc_init_ids(&shm_ids(ns));
+
+	/*
+	 * For init_ipc_ns shm_ids().rw_mutex is statically initialized
+	 * as kernel threads should be able to use it in do_exit() before
+	 * shm_init(), which is called on do_initcall()
+	 */
+	if (ns == &init_ipc_ns)
+		__ipc_init_ids(&shm_ids(ns));
+	else
+		ipc_init_ids(&shm_ids(ns));
 }
 
 /*
diff -puN ipc/util.c~shm-fix-a-race-between-shm_exit-and-shm_init ipc/util.c
--- a/ipc/util.c~shm-fix-a-race-between-shm_exit-and-shm_init
+++ a/ipc/util.c
@@ -108,31 +108,35 @@ static int __init ipc_init(void)
 }
 __initcall(ipc_init);
 
-/**
- *	ipc_init_ids		-	initialise IPC identifiers
- *	@ids: Identifier set
- *
- *	Set up the sequence range to use for the ipc identifier range (limited
- *	below IPCMNI) then initialise the ids idr.
- */
- 
-void ipc_init_ids(struct ipc_ids *ids)
+void __ipc_init_ids(struct ipc_ids *ids)
 {
-	init_rwsem(&ids->rw_mutex);
-
 	ids->in_use = 0;
 	ids->seq = 0;
 	{
 		int seq_limit = INT_MAX/SEQ_MULTIPLIER;
 		if (seq_limit > USHRT_MAX)
 			ids->seq_max = USHRT_MAX;
-		 else
-		 	ids->seq_max = seq_limit;
+		else
+			ids->seq_max = seq_limit;
 	}
 
 	idr_init(&ids->ipcs_idr);
 }
 
+/**
+ *	ipc_init_ids		-	initialise IPC identifiers
+ *	@ids: Identifier set
+ *
+ *	Set up the sequence range to use for the ipc identifier range (limited
+ *	below IPCMNI) then initialise the ids idr.
+ */
+
+void ipc_init_ids(struct ipc_ids *ids)
+{
+	init_rwsem(&ids->rw_mutex);
+	__ipc_init_ids(ids);
+}
+
 #ifdef CONFIG_PROC_FS
 static const struct file_operations sysvipc_proc_fops;
 /**
diff -puN ipc/util.h~shm-fix-a-race-between-shm_exit-and-shm_init ipc/util.h
--- a/ipc/util.h~shm-fix-a-race-between-shm_exit-and-shm_init
+++ a/ipc/util.h
@@ -80,6 +80,7 @@ struct seq_file;
 struct ipc_ids;
 
 void ipc_init_ids(struct ipc_ids *);
+void __ipc_init_ids(struct ipc_ids *ids);
 #ifdef CONFIG_PROC_FS
 void __init ipc_init_proc_interface(const char *path, const char *header,
 		int ids, int (*show)(struct seq_file *, void *));
_

Patches currently in -mm which might be from segoon@openwall.com are

origin.patch
taskstats-add_del_listener-shouldnt-use-the-wrong-node.patch
taskstats-add_del_listener-should-ignore-valid-listeners.patch
shm-fix-a-race-between-shm_exit-and-shm_init.patch
linux-next.patch
arch-arm-mach-ux500-mbox-db5500c-world-writable-sysfs-fifo-file.patch
ipc-introduce-shm_rmid_forced-sysctl-testing.patch