Re: netns: use a spin_lock to protect nsid management

Re: netns: use a spin_lock to protect nsid management

[Cong Wang]

(Please always Cc netdev for networking bugs)

On Tue, May 12, 2015 at 10:52 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> Hello Nicolas Dichtel,
>
> The patch 95f38411df05: "netns: use a spin_lock to protect nsid
> management" from May 7, 2015, leads to the following static checker
> warning:
>
>         net/core/net_namespace.c:580 rtnl_net_newid()
>         warn: inconsistent returns 'spin_lock:&nsid_lock'.
>
> net/core/net_namespace.c
>    565
>    566          spin_lock_irqsave(&nsid_lock, flags);
>    567          if (__peernet2id(net, peer) >= 0) {
>    568                  err = -EEXIST;
>    569                  goto out;
>
> I don't know if __peernet2id() unlocks on error but it can't possibly
> restore flags so this isn't right.
>

Yes it is a bug.

Also why do we have to disable IRQ when holding the spinlock?
IOW, which case takes the spin lock in IRQ context?

Thanks.

Re: netns: use a spin_lock to protect nsid management

[Nicolas Dichtel]

Le 12/05/2015 19:52, Dan Carpenter a écrit :
> Hello Nicolas Dichtel,
>
> The patch 95f38411df05: "netns: use a spin_lock to protect nsid
> management" from May 7, 2015, leads to the following static checker
> warning:
>
> 	net/core/net_namespace.c:580 rtnl_net_newid()
> 	warn: inconsistent returns 'spin_lock:&nsid_lock'.
>
> net/core/net_namespace.c
>     565
>     566          spin_lock_irqsave(&nsid_lock, flags);
>     567          if (__peernet2id(net, peer) >= 0) {
>     568                  err = -EEXIST;
>     569                  goto out;
>
> I don't know if __peernet2id() unlocks on error but it can't possibly
> restore flags so this isn't right.
It's a bug. I will send a patch.

Thank you for the report.

Nicolas

[PATCH net-next] netns: fix unbalanced spin_lock on error

[Nicolas Dichtel]

Unlock was missing on error path.

Fixes: 95f38411df05 ("netns: use a spin_lock to protect nsid management")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/net_namespace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a665bf490c88..cb32c0378c10 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -565,6 +565,7 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 	spin_lock_irqsave(&nsid_lock, flags);
 	if (__peernet2id(net, peer) >= 0) {
+		spin_unlock_irqrestore(&nsid_lock, flags);
 		err = -EEXIST;
 		goto out;
 	}
-- 
2.2.2

Re: netns: use a spin_lock to protect nsid management

[Nicolas Dichtel]

Le 12/05/2015 21:16, Cong Wang a écrit :
[snip]
> Also why do we have to disable IRQ when holding the spinlock?
> IOW, which case takes the spin lock in IRQ context?
peernet2id() is called by do_one_broadcast() (net/netlink/af_netlink.c). So
every callers of netlink_broadcast[_filtered]()/nlmsg_unicast()
/nlmsg_multicast() may needs this lock.

Re: [PATCH net-next] netns: fix unbalanced spin_lock on error

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Wed, 13 May 2015 13:43:09 +0200

> Unlock was missing on error path.
> 
> Fixes: 95f38411df05 ("netns: use a spin_lock to protect nsid management")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied.