[RFC][PATCH] Make access to taks's nsproxy liter

[RFC][PATCH] Make access to taks's nsproxy liter

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

Signed-off-by: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>

---

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

diff --git a/fs/proc/base.c b/fs/proc/base.c
index ed2b224..614851a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -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);
 		}
 
diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
index 525d8fc..74f21fe 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -32,8 +32,14 @@ struct nsproxy {
 };
 extern struct nsproxy init_nsproxy;
 
+static inline struct nsproxy *task_nsproxy(struct task_struct *tsk)
+{
+	return rcu_dereference(tsk->nsproxy);
+}
+
 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
diff --git a/kernel/exit.c b/kernel/exit.c
index 2f4f35e..a2aef1f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -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;
diff --git a/kernel/fork.c b/kernel/fork.c
index bc48e0f..0c6dd67 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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;
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index 58843ae..91dca27 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -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);
+
+	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);
+	}
+}
+
+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),

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Eric W. Biederman]

Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> writes:

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


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

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/08, Pavel Emelyanov wrote:
>
> 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.

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?

--- kernel/fork.c	2007-07-28 16:58:17.000000000 +0400
+++ /proc/self/fd/0	2007-08-08 20:30:33.325216944 +0400
@@ -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).

> +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
> +		 */
> +		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.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

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

On 08/08, Pavel Emelyanov wrote:
>
> 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.

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?

--- kernel/fork.c	2007-07-28 16:58:17.000000000 +0400
+++ /proc/self/fd/0	2007-08-08 20:30:33.325216944 +0400
@@ -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).

> +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
> +		 */
> +		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.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Serge E. Hallyn]

Quoting Pavel Emelyanov (xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org):
> 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 <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> 
> ---
> 
> 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(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ed2b224..614851a 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -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);
> 		}
> 
> diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
> index 525d8fc..74f21fe 100644
> --- a/include/linux/nsproxy.h
> +++ b/include/linux/nsproxy.h
> @@ -32,8 +32,14 @@ struct nsproxy {
> };
> extern struct nsproxy init_nsproxy;
> 
> +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)

> 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
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 2f4f35e..a2aef1f 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -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;
> diff --git a/kernel/fork.c b/kernel/fork.c
> index bc48e0f..0c6dd67 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -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;
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index 58843ae..91dca27 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -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);
> +
> +	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);
> +	}
> +}

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

thanks,
-serge

> +
> +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),

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/08, Serge E. Hallyn wrote:
>
> Quoting Pavel Emelyanov (xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org):
> > +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
> > +		 */
> > +		synchronize_rcu();
> > +		free_nsproxy(ns);
> > +	}
> > +}
> 
> Also a comment above switch_task_namespaces() that it must be called
> with task_lock held.

Heh, indeed, sys_unshare() does this. might_sleep() won't be happy.

Oleg.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Eric W. Biederman]

Oleg Nesterov <oleg-6lXkIZvqkOAvJsYlp49lxw@public.gmane.org> writes:

> On 08/08, Pavel Emelyanov wrote:
>>
>> 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.
>
> 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?
>
> --- kernel/fork.c	2007-07-28 16:58:17.000000000 +0400
> +++ /proc/self/fd/0	2007-08-08 20:30:33.325216944 +0400
> @@ -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).
>
>> +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
>> +		 */
>> +		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.

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?

Eric

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/08, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg-6lXkIZvqkOAvJsYlp49lxw@public.gmane.org> writes:
> 
> > On 08/08, Pavel Emelyanov wrote:
> >>
> >> +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
> >> +		 */
> >> +		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.
> 
> 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.

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

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.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Paul E. McKenney]

On Wed, Aug 08, 2007 at 08:41:07PM +0400, Oleg Nesterov wrote:
> This time Paul E. McKenney actually cc'ed, sorry for the extra
> noise...
> 
> On 08/08, Pavel Emelyanov wrote:
> >
> > 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.
> 
> 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?
> 
> --- kernel/fork.c	2007-07-28 16:58:17.000000000 +0400
> +++ /proc/self/fd/0	2007-08-08 20:30:33.325216944 +0400
> @@ -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).
> 
> > +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
> > +		 */
> > +		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.

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

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/08, Paul E. McKenney wrote:
>
> On Wed, Aug 08, 2007 at 08:41:07PM +0400, Oleg Nesterov wrote:
> > > +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
> > > +		 */
> > > +		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.
> 
> 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.

Oleg.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Paul E. McKenney]

On Wed, Aug 08, 2007 at 09:36:47PM +0400, Oleg Nesterov wrote:
> On 08/08, Paul E. McKenney wrote:
> >
> > On Wed, Aug 08, 2007 at 08:41:07PM +0400, Oleg Nesterov wrote:
> > > > +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
> > > > +		 */
> > > > +		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.
> > 
> > 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.

Makes sense.  Just for completeness, the other thing you could do would
be to do both a synchronize_sched() and a synchronize_rcu() in the
switch_task_namespaces() function, but I believe that Eric's approach
would be better.  (The only counter-example I can come up with off-hand
would be if there were tons of read paths, and you needed a quick fix.
But even in that case, hopefully the quick fix would be followed by
taking Eric's approach.)

						Thanx, Paul

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

Oleg Nesterov wrote:
> On 08/08, Eric W. Biederman wrote:
>> Oleg Nesterov <oleg-6lXkIZvqkOAvJsYlp49lxw@public.gmane.org> writes:
>>
>>> On 08/08, Pavel Emelyanov wrote:
>>>> +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
>>>> +		 */
>>>> +		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.

void __lockfunc _write_lock(rwlock_t *lock)
{
        preempt_disable();
        rwlock_acquire(&lock->dep_map, 0, 0, _RET_IP_);
        LOCK_CONTENDED(lock, _raw_write_trylock, _raw_write_lock);
}

preempt_disable == rcu_read_lock() due to
#define rcu_read_lock() \
        do { \
                preempt_disable(); \
                __acquire(RCU); \
        } while(0)

so currently this is enough to write_lock()

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

we can safely access parent's nsproxy with this patch like this:

rcu_read_lock();
nsproxy = task_nsproxy(p->parent);
BUG_ON(nsproxy == NULL); /* parent should reparent us before exiting nsproxy */
pid_ns = nsproxy->pid_ns;
...
rcu_read_unlock();


> 
> Oleg.
> 
> 

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

Eric W. Biederman wrote:
> Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> writes:
> 
>> 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.
> 
> 
> 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!

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

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

[snip]

>> diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
>> index 525d8fc..74f21fe 100644
>> --- a/include/linux/nsproxy.h
>> +++ b/include/linux/nsproxy.h
>> @@ -32,8 +32,14 @@ struct nsproxy {
>> };
>> extern struct nsproxy init_nsproxy;
>>
>> +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]

>> +	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
>> +		 */
>> +		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().

> thanks,
> -serge

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

Oleg Nesterov wrote:
> This time Paul E. McKenney actually cc'ed, sorry for the extra
> noise...
> 
> On 08/08, Pavel Emelyanov wrote:
>> 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.
> 
> 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?
> 
> --- kernel/fork.c	2007-07-28 16:58:17.000000000 +0400
> +++ /proc/self/fd/0	2007-08-08 20:30:33.325216944 +0400
> @@ -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).

We may, but the intention of this patch is just (!) to make the access 
to other's task namespaces lockless. Solving the accessing parent's
nsproxy in do_notify_parent() is a (good) side effect :)

>> +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
>> +		 */
>> +		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.
> 
> 

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

Paul E. McKenney wrote:
> On Wed, Aug 08, 2007 at 08:41:07PM +0400, Oleg Nesterov wrote:
>> This time Paul E. McKenney actually cc'ed, sorry for the extra
>> noise...
>>
>> On 08/08, Pavel Emelyanov wrote:
>>> 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.
>> 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?
>>
>> --- kernel/fork.c	2007-07-28 16:58:17.000000000 +0400
>> +++ /proc/self/fd/0	2007-08-08 20:30:33.325216944 +0400
>> @@ -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).
>>
>>> +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
>>> +		 */
>>> +		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.
> 
> Can you use synchronize_sched() instead?  The synchronize_sched()

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

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

> Does this work?
> 
> 						Thanx, Paul
> 

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/09, Pavel Emelyanov wrote:
>
> Paul E. McKenney wrote:
> >On Wed, Aug 08, 2007 at 08:41:07PM +0400, Oleg Nesterov wrote:
> >>
> >>>+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
> >>>+		 */
> >>>+		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.
> >
> >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().

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

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

Oleg Nesterov wrote:
> On 08/09, Pavel Emelyanov wrote:
>> Paul E. McKenney wrote:
>>> On Wed, Aug 08, 2007 at 08:41:07PM +0400, Oleg Nesterov wrote:
>>>>> +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
>>>>> +		 */
>>>>> +		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.
>>> 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().

Ok, thanks.

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

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.

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

I've moved this lower :)

> Oleg.
> 
> 

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/09, Oleg Nesterov wrote:
>
> Note also that switch_task_namespaces() might_sleep(), but sys_unshare()
> calls it under task_lock().

Ah, sorry, didn't notice your patch moves task_lock() down in sys_unshare().

Oleg.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Eric W. Biederman]

Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> writes:

> Eric W. Biederman wrote:
>> Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> writes:
>>
>>> 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.
>>
>>
>> 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

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Oleg Nesterov]

On 08/09, Pavel Emelyanov wrote:
>
> Oleg Nesterov wrote:
> >
> >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.

Ah yes, you are right. This is sad, because now I have no "hard" argument
against this patch :) Except "complicates" may be...

Oleg.

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Pavel Emelyanov]

Eric W. Biederman wrote:
> Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> writes:
> 
>> Eric W. Biederman wrote:
>>> Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org> writes:
>>>
>>>> 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.
>>>
>>> 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.

Oh! Ok, I will add it. Thanks.

> Eric
> 

Re: [RFC][PATCH] Make access to taks's nsproxy liter

[Serge E. Hallyn]

Quoting Pavel Emelyanov (xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org):
> [snip]
> 
> >>diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
> >>index 525d8fc..74f21fe 100644
> >>--- a/include/linux/nsproxy.h
> >>+++ b/include/linux/nsproxy.h
> >>@@ -32,8 +32,14 @@ struct nsproxy {
> >>};
> >>extern struct nsproxy init_nsproxy;
> >>
> >>+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.

Yeah...  as i said...  but I still get people asking "what is rcu
anyway" so don't think we can assume the implication is clear to
everyone.

> [snip]
> 
> >>+	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
> >>+		 */
> >>+		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().

Hmm,

Yes, I see.  And in the current usages it's correct.  I just worry that
the function takes the task_struct as an arg (which I know it must for
do_exit()) and so someone else might use it unlocked to switch another
task's namespace, for instance in an attempt to implement namespace
entering.

So ok "a comment .. that it must be called with task_lock held" would be
wrong, but please do add the *correct* comment :)  Namely that apart
from current usage in do_exit, this should not be used to switch nsproxy
on any task but current.  (Unless I'm still mistaken of course)

thanks,
-serge