Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan <adobriyan@gmail.com>]

On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
> David Miller a écrit :
>> From: Eric Dumazet <dada1@cosmosbay.com>
>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>
>>> This is better because :
>>>
>>> 1) No #ifdef CONFIG_NET_NS
>>>
>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>    centralized in the right file : include/net/net_namespace.h
>>
>> I %100 agree.
>
> Speaking of those functions, what do you think of this one ?
>
> static inline
> void dev_net_set(struct net_device *dev, struct net *net)
> {
> #ifdef CONFIG_NET_NS
>        release_net(dev->nd_net);
>        dev->nd_net = hold_net(net);
> #endif
> }
>
> I believe that its safer to hold a reference on "new" *before*
> releasing reference on "old" object.
>
> Also, release_net() and hold_net() can be defined to do
> the use_count refcounting regardless of CONFIG_NET_NS
> (Its a different NETNS_REFCNT_DEBUG #ifdef)

NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
is never freed.

> Yet another example where read_pnet() and write_pnet()
> are the right answer : Its cleaner and fixes *bugs*.

pnet stuff by definition can't fix bugs :-)

Ask from where new net comes?

If from current, nsproxy refcount is at least 1, so netns refcount is at least 1,
so shutdown sequence can't start.

If from userspace socket, there is task in netns -- see #1

If from netdevice on which ioctl is done, some task did ioctl -- see #1.

And so on.

But this is peanuts, because your race matters only when netns is almost freed
(in kmem_cache_free sense), so you're stashing dangling pointer somewhere else
which is a bug by itself.

> static inline
> void dev_net_set(struct net_device *dev, struct net *net)
> {
> 	hold_net(net);
> 	release_net(read_pnet(&dev->nd_net);
>        write_pnet(&dev->nd_net, net);
> }