* re: netns: use a spin_lock to protect nsid management
@ 2015-05-12 17:52 Dan Carpenter
2015-05-12 19:16 ` Cong Wang
2015-05-13 11:37 ` Nicolas Dichtel
0 siblings, 2 replies; 11+ messages in thread
From: Dan Carpenter @ 2015-05-12 17:52 UTC (permalink / raw)
To: kernel-janitors
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.
570 }
571
572 err = alloc_netid(net, peer, nsid);
573 spin_unlock_irqrestore(&nsid_lock, flags);
574 if (err >= 0) {
575 rtnl_net_notifyid(net, RTM_NEWNSID, err);
576 err = 0;
577 }
578 out:
579 put_net(peer);
580 return err;
581 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: netns: use a spin_lock to protect nsid management
2015-05-12 17:52 netns: use a spin_lock to protect nsid management Dan Carpenter
@ 2015-05-12 19:16 ` Cong Wang
2015-05-13 11:37 ` Nicolas Dichtel
1 sibling, 0 replies; 11+ messages in thread
From: Cong Wang @ 2015-05-12 19:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Nicolas Dichtel, kernel-janitors, Linux Kernel Network Developers
(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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: netns: use a spin_lock to protect nsid management
@ 2015-05-12 19:16 ` Cong Wang
0 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2015-05-12 19:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Nicolas Dichtel, kernel-janitors, Linux Kernel Network Developers
(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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: netns: use a spin_lock to protect nsid management
2015-05-12 19:16 ` Cong Wang
@ 2015-05-13 11:58 ` Nicolas Dichtel
-1 siblings, 0 replies; 11+ messages in thread
From: Nicolas Dichtel @ 2015-05-13 11:58 UTC (permalink / raw)
To: Cong Wang, Dan Carpenter; +Cc: kernel-janitors, Linux Kernel Network Developers
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.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: netns: use a spin_lock to protect nsid management
2015-05-12 17:52 netns: use a spin_lock to protect nsid management Dan Carpenter
@ 2015-05-13 11:37 ` Nicolas Dichtel
2015-05-13 11:37 ` Nicolas Dichtel
1 sibling, 0 replies; 11+ messages in thread
From: Nicolas Dichtel @ 2015-05-13 11:37 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, netdev
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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: netns: use a spin_lock to protect nsid management
@ 2015-05-13 11:37 ` Nicolas Dichtel
0 siblings, 0 replies; 11+ messages in thread
From: Nicolas Dichtel @ 2015-05-13 11:37 UTC (permalink / raw)
To: Dan Carpenter; +Cc: kernel-janitors, netdev
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH net-next] netns: fix unbalanced spin_lock on error
2015-05-13 11:37 ` Nicolas Dichtel
@ 2015-05-13 11:43 ` Nicolas Dichtel
-1 siblings, 0 replies; 11+ messages in thread
From: Nicolas Dichtel @ 2015-05-13 11:43 UTC (permalink / raw)
To: dan.carpenter; +Cc: davem, netdev, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH net-next] netns: fix unbalanced spin_lock on error
@ 2015-05-13 11:43 ` Nicolas Dichtel
0 siblings, 0 replies; 11+ messages in thread
From: Nicolas Dichtel @ 2015-05-13 11:43 UTC (permalink / raw)
To: dan.carpenter; +Cc: davem, netdev, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH net-next] netns: fix unbalanced spin_lock on error
2015-05-13 11:43 ` Nicolas Dichtel
@ 2015-05-15 2:36 ` David Miller
-1 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2015-05-15 2:36 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: dan.carpenter, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH net-next] netns: fix unbalanced spin_lock on error
@ 2015-05-15 2:36 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2015-05-15 2:36 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: dan.carpenter, netdev, kernel-janitors
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-05-15 2:36 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-12 17:52 netns: use a spin_lock to protect nsid management Dan Carpenter
2015-05-12 19:16 ` Cong Wang
2015-05-12 19:16 ` Cong Wang
2015-05-13 11:58 ` Nicolas Dichtel
2015-05-13 11:58 ` Nicolas Dichtel
2015-05-13 11:37 ` Nicolas Dichtel
2015-05-13 11:37 ` Nicolas Dichtel
2015-05-13 11:43 ` [PATCH net-next] netns: fix unbalanced spin_lock on error Nicolas Dichtel
2015-05-13 11:43 ` Nicolas Dichtel
2015-05-15 2:36 ` David Miller
2015-05-15 2:36 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.