lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 10 Sep 2007 13:30:46 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	"Serge E. Hallyn" <serue@...ibm.com>
Cc:	David Miller <davem@...emloft.net>,
	Linux Containers <containers@...ts.osdl.org>,
	netdev@...r.kernel.org
Subject: Re: [PATCH 16/16] net: netlink support for moving devices between network namespaces.

"Serge E. Hallyn" <serue@...ibm.com> writes:
>> 
>> +static struct net *get_net_ns_by_pid(pid_t pid)
>> +{
>> +	struct task_struct *tsk;
>> +	struct net *net;
>> +
>> +	/* Lookup the network namespace */
>> +	net = ERR_PTR(-ESRCH);
>> +	rcu_read_lock();
>> +	tsk = find_task_by_pid(pid);
>> +	if (tsk) {
>> +		task_lock(tsk);
>> +		if (tsk->nsproxy)
>> +			net = get_net(tsk->nsproxy->net_ns);
>> +		task_unlock(tsk);
>
> Thinking...  Ok, I'm not sure this is 100% safe in the target tree, but
> the long-term correct way probably isn't yet implemented in the net-
> tree.  Eventually you will want to:
>
> 	net_ns = NULL;
> 	rcu_read_lock();
> 	tsk = find_task_by_pid();  /* or _pidns equiv? */
> 	nsproxy = task_nsproxy(tsk);
> 	if (nsproxy)
> 		net_ns = get_net(nsproxy->net_ns);
> 	rcu_read_unlock;
>
> What you have here is probably unsafe if tsk is the last task pointing
> to it's nsproxy and it does an unshare, bc unshare isn't protected by
> task_lock, and you're not rcu_dereferencing tsk->nsproxy (which
> task_nsproxy does).  At one point we floated a patch to reuse the same
> nsproxy in that case which would prevent you having to worry about it,
> but that isn't being done in -mm now so i doubt it's in -net.


That change isn't merged upstream yet, so it isn't in David's
net-2.6.24 tree.  Currently task->nsproxy is protected but
task_lock(current). So the code is fine.

I am aware that removing the task_lock(current) for the setting
of current->nsproxy is currently in the works, and I have planned
to revisit this later when all of these pieces come together.

For now the code is fine.

If need be we can drop this patch to remove the potential merge
conflict.  But I figured it was useful for this part of the user space
interface to be available for review.

Eric
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists