Re: [RFC PATCH linux-next] ns: do not allocate a new nsproxy at eachcall

[Guillaume Gaudonville]

On 10/15/2013 08:40 PM, Eric W. Biederman wrote:
> "Guillaume Gaudonville" <gaudonville@xxxxxxxxx> writes:
>
> > Currently, at each call of setns system call a new nsproxy is allocated,
> > the old nsproxy namespaces are copied into the new one and the old nsproxy
> > is freed if the task was the only one to use it.
> >
> > It can creates large delays on hardware with large number of cpus since
> > to free a nsproxy a synchronize_rcu() call is done.
> >
> > When a task is the only one to use a nsproxy, only the task can do an action
> > that will make this nsproxy to be shared by another task or thread (fork,...).
> > So when the refcount of the nsproxy is equal to 1, we can simply update the
> > current nsproxy field without allocating a new one and freeing the old one.
> >
> > The install operations of each kind of namespace cannot fails, so there's no
> > need to check for an error and calling ops->install().
> >
> > Tested on TileGX (36 cores) and Intel (32 cores).
> This may be worth doing (I am a little scared of a design that has setns
> on a fast path) but right now this isn't safe.
>
> Currently pidns_install ends with:
>
> 	put_pid_ns(nsproxy->pid_ns_for_children);
> 	nsproxy->pid_ns_for_children = get_pid_ns(new);
> 	return 0;
>
>
> And netns_install ends with:
>
> 	put_net(nsproxy->net_ns);
> 	nsproxy->net_ns = get_net(net);
> 	return 0;
>
> The put before the set is not atomic and is not safe unless
> the nsproxy is private.  I think this is fixable but it requires a more
> indepth look at the code than you have done.
My expectation was that nobody else but the task itself could increase the
nsproxy refcount (ie. use it), if the refcount was equal to 1. So there
was no possible races while playing with nsproxy in that case.
Do you mean that someone else than the task itself could play with the 
nsproxy,
even if its refcount is equal to 1?

> Mind if I ask where this comes up?
The issue has been seen on a daemon that is performing monitoring and
configuration tasks in different netns.
> > Reported-by: Chris Metcalf <cmetcalf@xxxxxxxxxx>
> > Signed-off-by: Guillaume Gaudonville <guillaume.gaudonville@xxxxxxxxx>
> > ---
> >   kernel/nsproxy.c |   12 ++++++++++++
> >   1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> > index afc0456..afc04ac 100644
> > --- a/kernel/nsproxy.c
> > +++ b/kernel/nsproxy.c
> > @@ -255,6 +255,18 @@ SYSCALL_DEFINE2(setns, int, fd, int, nstype)
> >   	if (nstype && (ops->type != nstype))
> >   		goto out;
> >   
> > +	/*
> > +	 * If count == 1, only the current task can increment it,
> > +	 * by doing a fork for example so we can safely update the
> > +	 * current nsproxy pointers without allocate a new one,
> > +	 * update it and destroy the old one
> > +	 */
> > +	if (atomic_read(&tsk->nsproxy->count) == 1) {
> > +		err = ops->install(tsk->nsproxy, ei->ns);
> > +		fput(file);
> > +		return err;
> > +	}
> As a minor nit, but to match the rest of the code in this function that
> should read:
>
> > +	if (atomic_read(&tsk->nsproxy->count) == 1) {
> > +		err = ops->install(tsk->nsproxy, ei->ns);
> > +		goto out;
> > +	}
> There is no need to add an additional exit point to reason about.
>
> > +
> >   	new_nsproxy = create_new_namespaces(0, tsk, current_user_ns(), tsk->fs);
> >   	if (IS_ERR(new_nsproxy)) {
> >   		err = PTR_ERR(new_nsproxy);


--
Guillaume Gaudonville
6WIND
Software Engineer

Tel: +33 1 39 30 92 53
Mob: +33 6 47 85 34 33
Fax: +33 1 39 30 92 11
guillaume.gaudonville@xxxxxxxxx
www.6wind.com
Join the Multicore Packet Processing Forum: www.multicorepacketprocessing.com

Ce courriel ainsi que toutes les pièces jointes, est uniquement destiné à son ou ses destinataires. Il contient des informations confidentielles qui sont la propriété de 6WIND. Toute révélation, distribution ou copie des informations qu'il contient est strictement interdite. Si vous avez reçu ce message par erreur, veuillez immédiatement le signaler à l'émetteur et détruire toutes les données reçues

This e-mail message, including any attachments, is for the sole use of the intended recipient(s) and contains information that is confidential and proprietary to 6WIND. All unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply e-mail and destroy all copies of the original message.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/