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: + 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.

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()

[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: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: [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.