[PATCH bluetooth-next] nl802154: fix rtnl_unlock() being missing

[PATCH bluetooth-next] nl802154: fix rtnl_unlock() being missing

[Jεan Sacren]

From: Jean Sacren <sakiwit@gmail.com>

In nl802154_prepare_wpan_dev_dump(), rtnl_unlock() was missing if it
returns 0. If we insert rtnl_unlock() mechanically, we will have
unbearable code duplication. Fix this bug by unifying exit paths.

Following the new exit path, clean up now the obsolete goto statement.

Fixes: a26c5fd7622d ("nl802154: add support for security layer")
Signed-off-by: Jean Sacren <sakiwit@gmail.com>
Cc: Alexander Aring <alex.aring@gmail.com>
---
 net/ieee802154/nl802154.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 1e9e86508441..0074f13ea4ca 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -253,7 +253,7 @@ nl802154_prepare_wpan_dev_dump(struct sk_buff *skb,
 			       struct cfg802154_registered_device **rdev,
 			       struct wpan_dev **wpan_dev)
 {
-	int err;
+	int err = 0;
 
 	rtnl_lock();
 
@@ -293,13 +293,10 @@ nl802154_prepare_wpan_dev_dump(struct sk_buff *skb,
 			}
 		}
 
-		if (!*wpan_dev) {
+		if (!*wpan_dev)
 			err = -ENODEV;
-			goto out_unlock;
-		}
 	}
 
-	return 0;
  out_unlock:
 	rtnl_unlock();
 	return err;

Re: [PATCH bluetooth-next] nl802154: fix rtnl_unlock() being missing

[Alexander Aring]

Hi Jean,

On Fri, Oct 02, 2015 at 10:46:52PM -0600, Jεan Sacren wrote:
> From: Jean Sacren <sakiwit@gmail.com>
> 
> In nl802154_prepare_wpan_dev_dump(), rtnl_unlock() was missing if it

There is no rtnl_unlock missing, the rtnl_unlock call will be done by
function "nl802154_finish_wpan_dev_dump".

The rtnl lock need to be held at the full netlink _dump_ callback, e.g.:

1. call nl802154_prepare_wpan_dev_dump

2. doing netlink things

3. nl802154_finish_wpan_dev_dump

The 3. point is important and we have a bug if we don't call
"nl802154_finish_wpan_dev_dump" at least. Except
"nl802154_prepare_wpan_dev_dump" returns an error.


This mechanism was copy&pasted(with minor changes) from wireless which
we use as reference design. If you like to change this behaviour, then
you need to first change it in wireless, then here.

The identical function can be found at [0].

btw:
Doing some shared code between wireless/802.15.4 would be great but at
first we try to reach some stable subsystem implementation.

- Alex

[0] http://lxr.free-electrons.com/source/net/wireless/nl80211.c#L481

Re: [PATCH bluetooth-next] nl802154: fix rtnl_unlock() being missing

[Alexander Aring]

On Sat, Oct 03, 2015 at 08:44:13AM +0200, Alexander Aring wrote:
> Hi Jean,
> 
> On Fri, Oct 02, 2015 at 10:46:52PM -0600, Jεan Sacren wrote:
> > From: Jean Sacren <sakiwit@gmail.com>
> > 
> > In nl802154_prepare_wpan_dev_dump(), rtnl_unlock() was missing if it
> 
> There is no rtnl_unlock missing, the rtnl_unlock call will be done by
> function "nl802154_finish_wpan_dev_dump".
> 
> The rtnl lock need to be held at the full netlink _dump_ callback, e.g.:
> 
> 1. call nl802154_prepare_wpan_dev_dump
> 
> 2. doing netlink things
> 
> 3. nl802154_finish_wpan_dev_dump
> 

Actually we have something like that for the "doit" callback. In this case
netlink offers a "pre" and "post" callback for doing things before and
after the netlink call, see [0].

Maybe another idea is to have a "pre_dumpit" and "post_dumpit", but then
we need to care about dump callbacks which doesn't use
"nl802154_prepare_wpan_dev_dump" - these callbacks exist. I have not
exactly look at it and don't know how possible it is. It would be a nice
cleanup. Maybe this can be workarounded by some FLAG to do WDEV_PEPARE
or not. I don't know, just an idea. ;-)

But then do it at first in wireless subsystem. I also don't know if
the wireless community likes this idea or not.

The "pre" "post" ensures that we always handles the rtnl locks
correctly.

- Alex

[0]http://lxr.free-electrons.com/source/net/wireless/nl80211.c#L47

Re: [PATCH bluetooth-next] nl802154: fix rtnl_unlock() being missing

[Jεan Sacren]

From: Alexander Aring <alex.aring@gmail.com>
Date: Sat, 03 Oct 2015 08:44:14 +0200
>
> Hi Jean,
> 
> On Fri, Oct 02, 2015 at 10:46:52PM -0600, Jεan Sacren wrote:
> > From: Jean Sacren <sakiwit@gmail.com>
> > 
> > In nl802154_prepare_wpan_dev_dump(), rtnl_unlock() was missing if it
> 
> There is no rtnl_unlock missing, the rtnl_unlock call will be done by
> function "nl802154_finish_wpan_dev_dump".
> 
> The rtnl lock need to be held at the full netlink _dump_ callback, e.g.:
> 
> 1. call nl802154_prepare_wpan_dev_dump
> 
> 2. doing netlink things
> 
> 3. nl802154_finish_wpan_dev_dump

I'm so sorry. Thank you for teaching me again.