netdev - Re: [PATCH 16/16] net: netlink support for moving devices between network namespaces.

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 linux-hardening linux-cve-announce 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]

Message-ID: <20070911005404.GA19808@sergelap.austin.ibm.com>
Date:	Mon, 10 Sep 2007 19:54:04 -0500
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	"Serge E. Hallyn" <serue@...ibm.com>,
	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.

Quoting Eric W. Biederman (ebiederm@...ssion.com):
> "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.

No, no.  Like you say it's correct at the moment.  Just something we
need to watch out for when it does get merged with the newer changes.

> But I figured it was useful

Absolutely.

> for this part of the user space
> interface to be available for review.

Agreed.  And the rest of the patchset looks good to me.

Thanks.

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