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

PATCH New Oleg Nesterov 3 participants 13 messages Aug 3, 2011

Share & tools

Oleg Nesterov

author

2011-08-03 14:08 UTC #0 lore raw

Quoted 8 lines

From: Vasiliy Kulikov <[email protected]>
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.

Vasiliy Kulikov

2011-08-03 18:24 UTC #1 lore raw

Quoted 17 lines · Oleg Nesterov

Quoted 8 lines
From: Vasiliy Kulikov <[email protected]>
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);

Quoted 6 lines

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.

Signature · 2 lines

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

Vasiliy Kulikov

2011-08-03 18:27 UTC #2 lore raw

PATCH 1/1 shm: fix wrong tests

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

Signed-off-by Vasiliy Kulikov <[email protected]>

---
 ipc/shm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

ipc/shm.c +2 -2

index bf46636..4e3c883 100644
@@ -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);
 }

Signature · 1 line

1.7.0.4

Vasiliy Kulikov

2011-08-03 18:28 UTC #3 lore raw

PATCH 1/1 shm: optimize exit_shm()

In reply to 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 <[email protected]>

---
 ipc/shm.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

ipc/shm.c +3 -0

index 4e3c883..855ddc0 100644
@@ -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)

Signature · 1 line

1.7.0.4

Manuel Lauss

2011-08-03 19:08 UTC #4 lore raw

Re: [PATCH] shm: optimize exit_shm()

Hello,

Quoted 15 lines · 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 <[email protected]>
---
ipc/shm.c | 3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
ipc/shm.c +0 -0
index 4e3c883..855ddc0 100644

ipc/shm.c void exit_shm(struct task_struct *task) · line 312

Quoted 9 lines

ipc/shm.c +3 -0

@@ -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

Vasiliy Kulikov

2011-08-03 19:16 UTC #5 lore raw

Re: [PATCH] shm: optimize exit_shm()

Hi Manuel,

Quoted 5 lines · Manuel Lauss

Quoted 4 lines · 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().

[...]

Quoted 8 lines

Quoted 6 lines
+ 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.

Quoted 1 line

And this also fixes the oops.

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

Thanks,

Signature · 2 lines

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

Oleg Nesterov

author

2011-08-03 19:32 UTC #6 lore raw

Re: [PATCH] shm: optimize exit_shm()

Quoted 7 lines · Vasiliy Kulikov

Quoted 5 lines · Manuel Lauss
Quoted 4 lines
+
/* Destroy all already created segments, but not mapped yet */ down_write(&shm_ids(ns).rw_mutex);
if (shm_ids(ns).in_use)

Quoted 5 lines

Quoted 1 line
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?

Quoted 4 lines

Quoted 1 line
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.

Vasiliy Kulikov

2011-08-03 19:41 UTC #7 lore raw

Re: [PATCH] shm: optimize exit_shm()

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Vasiliy Kulikov
Quoted 5 lines · Manuel Lauss
Quoted 4 lines
+
/* Destroy all already created segments, but not mapped yet */ down_write(&shm_ids(ns).rw_mutex);
if (shm_ids(ns).in_use)

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Signature · 2 lines

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

Oleg Nesterov

author

2011-08-03 19:47 UTC #8 lore raw

Re: [PATCH] shm: optimize exit_shm()

Quoted 12 lines · Vasiliy Kulikov

Quoted 10 lines · Oleg Nesterov
Quoted 9 lines · Vasiliy Kulikov
Quoted 6 lines · Manuel Lauss
Quoted 4 lines
+
/* 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?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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.

Quoted 9 lines

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.

Oleg Nesterov

author

2011-08-03 19:24 UTC #9 lore raw

Re: [PATCH] shm: optimize exit_shm()

In reply to Vasiliy Kulikov

Quoted 3 lines · Vasiliy Kulikov

As .in_use may be changed to zero while current
process was sleeping in down_write(),

Yes,

Quoted 2 lines

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.

Vasiliy Kulikov

2011-08-03 19:35 UTC #10 lore raw

Re: [PATCH] shm: optimize exit_shm()

Quoted 4 lines · Oleg Nesterov

Quoted 2 lines
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.

Signature · 2 lines

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

Oleg Nesterov

author

2011-08-03 19:42 UTC #11 lore raw

Re: [PATCH] shm: optimize exit_shm()

Quoted 11 lines · Vasiliy Kulikov

Quoted 4 lines · Oleg Nesterov
Quoted 2 lines
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,

Quoted 1 line

"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.

Oleg Nesterov

author

2011-08-03 19:21 UTC #12 lore raw

In reply to Vasiliy Kulikov

Quoted 9 lines · Vasiliy Kulikov

Quoted 4 lines · Oleg Nesterov
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.