Make access to taks's nsproxy liter

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Quoted 11 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

Quoted 4 lines

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Looks pretty decent but we need to clearly say what lock you have to have to assign to the nsproxy pointer.

In this case it isn't so much of a lock as simply you must be the process so I think the code is ok. But it sufficiently subtle it probably needs to be spelled out.

Eric

Quoted 19 lines · Eric W. Biederman

Quoted 15 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Quoted 2 lines

Looks pretty decent but we need to clearly say what lock you have to have to assign to the nsproxy pointer.

no locks - only current is allowed to change its nsproxy!

Quoted 19 lines · Pavel Emelyanov

Quoted 18 lines · Eric W. Biederman

Quoted 15 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Quoted 3 lines

Quoted 2 lines

Looks pretty decent but we need to clearly say what lock you have to have to assign to the nsproxy pointer.

no locks - only current is allowed to change its nsproxy!

Exactly. Just need a comment or something to make that clear.

Eric

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Quoted 12 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

Quoted 4 lines

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Still it is a bit sad we slow down process's exit. Perhaps I missed some other ->nsproxy access, but can't we make a simpler patch?

proc/self/fd/0 renamed from fork.c +2 -0

@@ -1633,7 +1633,9 @@ asmlinkage long sys_unshare(unsigned lon
 
 		if (new_nsproxy) {
 			old_nsproxy = current->nsproxy;
+			read_lock(&tasklist_lock);
 			current->nsproxy = new_nsproxy;
+			read_unlock(&tasklist_lock);
 			new_nsproxy = old_nsproxy;
 		}
 

This way ->nsproxy is stable under task_lock() or write_lock(tasklist).

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Quoted 29 lines · Oleg Nesterov

Quoted 15 lines · Pavel Emelyanov

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

Quoted 3 lines

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

But we should be able to do:

write_lock_irq();
rcu_read_lock();
muck with other tasks nsproxy.
rcu_read_unlock();
write_unlock_irq();

Which would make rcu fine.

The real locking we have is that only a task is allowed to modify it's own nsproxy pointer. Other processes are not.

The practical question is how do we enable other processes to read a particular tasks nsproxy or something pointed to by it?

Quoted 29 lines · Eric W. Biederman

Quoted 27 lines · Oleg Nesterov

Quoted 14 lines · Pavel Emelyanov

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

Quoted 3 lines

Quoted 3 lines

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Quoted 9 lines

But we should be able to do:

write_lock_irq();
rcu_read_lock();
muck with other tasks nsproxy.
rcu_read_unlock();
write_unlock_irq();

Which would make rcu fine.

Yes sure. I just meant that the patch looks incomplete. But we didn't hear Paul yet, perhaps I'm just wrong.

Quoted 3 lines

The real locking we have is that only a task is allowed to modify it's own nsproxy pointer. Other processes are not.

Quoted 2 lines

The practical question is how do we enable other processes to read a particular tasks nsproxy or something pointed to by it?

task_lock(). The only problem we can't take it in do_notify_parent(), but if we add read_lock(tasklist) to sys_unshare, we can safely access ->parent->nsproxy.

Oleg.

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

This time Paul E. McKenney actually cc'ed, sorry for the extra noise...

Quoted 12 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

Quoted 4 lines

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Still it is a bit sad we slow down process's exit. Perhaps I missed some other ->nsproxy access, but can't we make a simpler patch?

proc/self/fd/0 renamed from fork.c +2 -0

@@ -1633,7 +1633,9 @@ asmlinkage long sys_unshare(unsigned lon
 
 		if (new_nsproxy) {
 			old_nsproxy = current->nsproxy;
+			read_lock(&tasklist_lock);
 			current->nsproxy = new_nsproxy;
+			read_unlock(&tasklist_lock);
 			new_nsproxy = old_nsproxy;
 		}
 

This way ->nsproxy is stable under task_lock() or write_lock(tasklist).

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Quoted 27 lines · Oleg Nesterov

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

Quoted 3 lines

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Can you use synchronize_sched() instead? The synchronize_sched() primitive will wait until all preempt/irq-disable code sequences complete. Therefore, it would wait for all write_lock_irq() code sequences to complete.

Quoted 29 lines · Paul E. McKenney

Quoted 27 lines · Oleg Nesterov

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

Quoted 8 lines

Quoted 3 lines

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Can you use synchronize_sched() instead? The synchronize_sched() primitive will wait until all preempt/irq-disable code sequences complete. Therefore, it would wait for all write_lock_irq() code sequences to complete.

Thanks Paul!

But we also need to cover the case when ->nsproxy is used under rcu_read_lock(), so if we go this way, we'd better add rcu_read_lock() to do_notify_parent.*() as Eric suggested.

Quoted 63 lines · Oleg Nesterov

This time Paul E. McKenney actually cc'ed, sorry for the extra noise...

Quoted 11 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

Quoted 4 lines

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Still it is a bit sad we slow down process's exit. Perhaps I missed some other ->nsproxy access, but can't we make a simpler patch?

proc/self/fd/0 renamed from fork.c +2 -0

@@ -1633,7 +1633,9 @@ asmlinkage long sys_unshare(unsigned lon
 
 		if (new_nsproxy) {
 			old_nsproxy = current->nsproxy;
+			read_lock(&tasklist_lock);
 			current->nsproxy = new_nsproxy;
+			read_unlock(&tasklist_lock);
 			new_nsproxy = old_nsproxy;
 		}
 

This way ->nsproxy is stable under task_lock() or write_lock(tasklist).

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

Quoted 3 lines

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Can you use synchronize_sched() instead? The synchronize_sched()

primitive will wait until all preempt/irq-disable code sequences complete. Therefore, it would wait for all write_lock_irq() code sequences to complete.

Does this work?

Thanx, Paul

Quoted 34 lines · Paul E. McKenney

Quoted 32 lines · Oleg Nesterov

Quoted 15 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

This is correct with the current implementation of RCU, but strictly speaking,
we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply
rcu_read_lock() in theory.

Quoted 1 line

Can you use synchronize_sched() instead? The synchronize_sched()

#define synchronize_sched() synchronize_rcu()
they are the same? what's the point?

Quoted 3 lines

primitive will wait until all preempt/irq-disable code sequences complete. Therefore, it would wait for all write_lock_irq() code sequences to complete.

But we don't need this. Iff we get the nsproxy under rcu_read_lock() all we need is to wait for RCU sections to complete.

Quoted 31 lines · Pavel Emelyanov

Quoted 30 lines · Paul E. McKenney

Quoted 29 lines · Oleg Nesterov

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Quoted 3 lines

Quoted 1 line

Can you use synchronize_sched() instead? The synchronize_sched()

#define synchronize_sched() synchronize_rcu()
they are the same? what's the point?

There are the same with the current implementation. RT kernel for example, has another, when preempt_disable() doesn't imply rcu_read_lock().

Quoted 5 lines

Quoted 3 lines

primitive will wait until all preempt/irq-disable code sequences complete. Therefore, it would wait for all write_lock_irq() code sequences to complete.

But we don't need this. Iff we get the nsproxy under rcu_read_lock() all we need is to wait for RCU sections to complete.

Yes. But this patch complicates the code and slows down group_exit. We don't

access non-current ->nsproxy so often afaics, and task_lock is cheap.

Note also that switch_task_namespaces() might_sleep(), but sys_unshare() calls it under task_lock().

Oleg.

Quoted 3 lines · Oleg Nesterov

Yes. But this patch complicates the code and slows down group_exit. We don't

Nope - it slows done the code only if the task exiting is the last one using the nsproxy. In other words - we slowdown the virtual server stop, not task exit. This is OK.

Note also that switch_task_namespaces() might_sleep(), but sys_unshare() calls it under task_lock().

This time Paul E. McKenney actually cc'ed, sorry for the extra noise...

Quoted 11 lines · Pavel Emelyanov

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

Quoted 4 lines

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Still it is a bit sad we slow down process's exit. Perhaps I missed some other ->nsproxy access, but can't we make a simpler patch?

proc/self/fd/0 renamed from fork.c +2 -0

@@ -1633,7 +1633,9 @@ asmlinkage long sys_unshare(unsigned lon
 
 		if (new_nsproxy) {
 			old_nsproxy = current->nsproxy;
+			read_lock(&tasklist_lock);
 			current->nsproxy = new_nsproxy;
+			read_unlock(&tasklist_lock);
 			new_nsproxy = old_nsproxy;
 		}
 

This way ->nsproxy is stable under task_lock() or write_lock(tasklist).

Quoted 14 lines

(quoted diff) +14 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep
+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

(I may be wrong, Paul cc'ed)

This is correct with the current implementation of RCU, but strictly speaking, we can't use synchronize_rcu() here, because write_lock_irq() doesn't imply rcu_read_lock() in theory.

Oleg.

When someone wants to deal with some other taks's namespaces it has to lock the task and then to get the desired namespace if the one exists. This is slow on read-only paths and may be impossible in some cases.

E.g. Oleg recently noticed a race between unshare() and the (just sent for review) pid namespaces - when the task notifies the parent it has to know the parent's namespace, but taking the task_lock() is impossible there - the code is under write locked tasklist lock.

On the other hand switching the namespace on task (daemonize) and releasing the namespace (after the last task exit) is rather rare operation and we can sacrifice its speed to solve the issues above.

Signed-off-by Pavel Emelyanov <[email protected]>

---

fs/proc/base.c          |   27 +++++++++++++++++----------
include/linux/nsproxy.h |   19 +++++++------------
kernel/exit.c           |    4 +---
kernel/fork.c           |   11 +++++------
kernel/nsproxy.c        |   38 ++++++++++++++++++++++++++++++--------
5 files changed, 60 insertions(+), 39 deletions(-)

fs/proc/base.c +17 -10

index ed2b224..614851a 100644
@@ -371,18 +371,21 @@ struct proc_mounts {
 static int mounts_open(struct inode *inode, struct file *file)
 {
 	struct task_struct *task = get_proc_task(inode);
+	struct nsproxy *nsp;
 	struct mnt_namespace *ns = NULL;
 	struct proc_mounts *p;
 	int ret = -EINVAL;
 
 	if (task) {
-		task_lock(task);
-		if (task->nsproxy) {
-			ns = task->nsproxy->mnt_ns;
+		rcu_read_lock();
+		nsp = task_nsproxy(task);
+		if (nsp) {
+			ns = nsp->mnt_ns;
 			if (ns)
 				get_mnt_ns(ns);
 		}
-		task_unlock(task);
+		rcu_read_unlock();
+
 		put_task_struct(task);
 	}
 

@@ -445,16 +448,20 @@ static int mountstats_open(struct inode 
 
 	if (!ret) {
 		struct seq_file *m = file->private_data;
+		struct nsproxy *nsp;
 		struct mnt_namespace *mnt_ns = NULL;
 		struct task_struct *task = get_proc_task(inode);
 
 		if (task) {
-			task_lock(task);
-			if (task->nsproxy)
-				mnt_ns = task->nsproxy->mnt_ns;
-			if (mnt_ns)
-				get_mnt_ns(mnt_ns);
-			task_unlock(task);
+			rcu_read_lock();
+			nsp = task_nsproxy(task);
+			if (nsp) {
+				mnt_ns = nsp->mnt_ns;
+				if (mnt_ns)
+					get_mnt_ns(mnt_ns);
+			}
+			rcu_read_unlock();
+
 			put_task_struct(task);
 		}
 

include/linux/nsproxy.h +0 -0

index 525d8fc..74f21fe 100644

include/linux/nsproxy.h +2 -12

 int copy_namespaces(unsigned long flags, struct task_struct *tsk);
-void get_task_namespaces(struct task_struct *tsk);
+void exit_task_namespaces(struct task_struct *tsk);
+void switch_task_namespaces(struct task_struct *tsk, struct nsproxy *new);
 void free_nsproxy(struct nsproxy *ns);
 int unshare_nsproxy_namespaces(unsigned long, struct nsproxy **,
 	struct fs_struct *);

@@ -45,17 +51,6 @@ static inline void put_nsproxy(struct ns
 	}
 }
 
-static inline void exit_task_namespaces(struct task_struct *p)
-{
-	struct nsproxy *ns = p->nsproxy;
-	if (ns) {
-		task_lock(p);
-		p->nsproxy = NULL;
-		task_unlock(p);
-		put_nsproxy(ns);
-	}
-}
-
 #ifdef CONFIG_CONTAINER_NS
 int ns_container_clone(struct task_struct *tsk);
 #else

kernel/exit.c +1 -3

index 2f4f35e..a2aef1f 100644
@@ -409,9 +409,7 @@ void daemonize(const char *name, ...)
 	current->fs = fs;
 	atomic_inc(&fs->count);
 
-	exit_task_namespaces(current);
-	current->nsproxy = init_task.nsproxy;
-	get_task_namespaces(current);
+	switch_task_namespaces(current, init_task.nsproxy);
 
 	exit_files(current);
 	current->files = init_task.files;

kernel/fork.c +5 -6

index bc48e0f..0c6dd67 100644
@@ -1618,7 +1618,7 @@ asmlinkage long sys_unshare(unsigned lon
 	struct mm_struct *mm, *new_mm = NULL, *active_mm = NULL;
 	struct files_struct *fd, *new_fd = NULL;
 	struct sem_undo_list *new_ulist = NULL;
-	struct nsproxy *new_nsproxy = NULL, *old_nsproxy = NULL;
+	struct nsproxy *new_nsproxy = NULL;
 
 	check_unshare_flags(&unshare_flags);
 

@@ -1647,14 +1647,13 @@ asmlinkage long sys_unshare(unsigned lon
 
 	if (new_fs ||  new_mm || new_fd || new_ulist || new_nsproxy) {
 
-		task_lock(current);
-
 		if (new_nsproxy) {
-			old_nsproxy = current->nsproxy;
-			current->nsproxy = new_nsproxy;
-			new_nsproxy = old_nsproxy;
+			switch_task_namespaces(current, new_nsproxy);
+			new_nsproxy = NULL;
 		}
 
+		task_lock(current);
+
 		if (new_fs) {
 			fs = current->fs;
 			current->fs = new_fs;

kernel/nsproxy.c +14 -8

index 58843ae..91dca27 100644
@@ -30,14 +30,6 @@ static inline void get_nsproxy(struct ns
 	atomic_inc(&ns->count);
 }
 
-void get_task_namespaces(struct task_struct *tsk)
-{
-	struct nsproxy *ns = tsk->nsproxy;
-	if (ns) {
-		get_nsproxy(ns);
-	}
-}
-
 /*
 * creates a copy of "orig" with refcount 1.
 */

@@ -205,6 +197,36 @@ out:
 	return err;
 }
 
+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+

kernel/nsproxy.c +6 -0

+
+void exit_task_namespaces(struct task_struct *p)
+{
+	switch_task_namespaces(p, NULL);
+}
+
 static int __init nsproxy_cache_init(void)
 {
 	nsproxy_cachep = kmem_cache_create("nsproxy", sizeof(struct nsproxy),

Quoted 19 lines · Pavel Emelyanov

(quoted diff) +19 -0

+void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+{
+	struct nsproxy *ns;
+
+	might_sleep();
+
+	ns = p->nsproxy;
+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+
+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep

Review

Quoted 5 lines

(quoted diff) +5 -0

+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

Also a comment above switch_task_namespaces() that it must be called with task_lock held.

Quoted 8 lines

include/linux/nsproxy.h +0 -0

index 525d8fc..74f21fe 100644
@@ -32,8 +32,14 @@ struct nsproxy {
 };
 extern struct nsproxy init_nsproxy;
 

include/linux/nsproxy.h struct nsproxy { · line 38

Quoted 4 lines

include/linux/nsproxy.h +4 -0

+static inline struct nsproxy *task_nsproxy(struct task_struct *tsk)
+{
+	return rcu_dereference(tsk->nsproxy);
+}

Looks like a very nice cleanup as well. But please add a comment above task_nsproxy() that it must be called under rcu_read_lock() or task_lock(task) (though I'll admit the rcu_dereference may make that obvious)

Quoted 12 lines

include/linux/nsproxy.h +12 -0

+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+
+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep

include/linux/nsproxy.h

Quoted 5 lines

include/linux/nsproxy.h +5 -0

+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

Also a comment above switch_task_namespaces() that it must be called with task_lock held.

thanks,
-serge

[snip]

Quoted 8 lines

Quoted 8 lines

include/linux/nsproxy.h +0 -0

index 525d8fc..74f21fe 100644
@@ -32,8 +32,14 @@ struct nsproxy {
 };
 extern struct nsproxy init_nsproxy;
 

Quoted 9 lines

include/linux/nsproxy.h struct nsproxy { · line 38

Quoted 4 lines

include/linux/nsproxy.h +4 -0

+static inline struct nsproxy *task_nsproxy(struct task_struct *tsk)
+{
+	return rcu_dereference(tsk->nsproxy);
+}

Looks like a very nice cleanup as well. But please add a comment above task_nsproxy() that it must be called under rcu_read_lock() or task_lock(task) (though I'll admit the rcu_dereference may make that obvious)

I will, but I think that rcu_dereference implies this. Anyway.

[snip]

Quoted 12 lines

Quoted 12 lines

include/linux/nsproxy.h +12 -0

+	if (ns == new)
+		return;
+
+	if (new)
+		get_nsproxy(new);
+	rcu_assign_pointer(p->nsproxy, new);
+
+	if (ns && atomic_dec_and_test(&ns->count)) {
+		/*
+		 * wait for others to get what they want from this
+		 * nsproxy. cannot release this nsproxy via the
+		 * call_rcu() since put_mnt_ns will want to sleep

Quoted 8 lines

include/linux/nsproxy.h

Quoted 5 lines

include/linux/nsproxy.h +5 -0

+		 */
+		synchronize_rcu();
+		free_nsproxy(ns);
+	}
+}

Also a comment above switch_task_namespaces() that it must be called with task_lock held.

no! no locks here! free_nsproxy() may sleep when putting mnt_ns and maybe some other. see - there's a hunk in sys_unshare that move the task_lock() after switch_task_namespaces().