mac802154: remove pib/mib locks and locking fixes

This patch fixes an issue to set address configuration with ioctl. Accessing the mib requires rtnl lock and the ndo_do_ioctl doesn't hold the rtnl lock while this callback is called. This patch do that manually.

Signed-off-by Alexander Aring <[email protected]>

Reported-by Matteo Petracca <[email protected]>

Reviewed-by Stefan Schmidt <[email protected]>

---
 net/mac802154/iface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

net/mac802154/iface.c +2 -2

index 91b75ab..2a58788 100644
@@ -62,8 +62,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		(struct sockaddr_ieee802154 *)&ifr->ifr_addr;
 	int err = -ENOIOCTLCMD;
 
-	ASSERT_RTNL();
-
+	rtnl_lock();
 	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {

@@ -90,6 +89,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
 			spin_unlock_bh(&sdata->mib_lock);
+			rtnl_unlock();
 			return -EBUSY;
 		}
 

Quoted 35 lines

This patch fixes an issue to set address configuration with ioctl. Accessing the mib requires rtnl lock and the ndo_do_ioctl doesn't hold the rtnl lock while this callback is called. This patch do that manually.

Signed-off-by Alexander Aring <[email protected]>

Reported-by Matteo Petracca <[email protected]>

Reviewed-by Stefan Schmidt <[email protected]>

---
net/mac802154/iface.c | 5 +++--

1 file changed, 3 insertions(+), 2 deletions(-)

net/mac802154/iface.c +2 -2

index 91b75ab..2a58788 100644
@@ -62,8 +62,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		(struct sockaddr_ieee802154 *)&ifr->ifr_addr;
 	int err = -ENOIOCTLCMD;
 
-	ASSERT_RTNL();
-
+	rtnl_lock();
 	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {

@@ -90,6 +89,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
 			spin_unlock_bh(&sdata->mib_lock);
+			rtnl_unlock();
 			return -EBUSY;
 		}
 

net/mac802154/iface.c mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 117

Quoted 6 lines

net/mac802154/iface.c +1 -0

@@ -112,6 +112,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 
 	spin_unlock_bh(&sdata->mib_lock);
+	rtnl_unlock();
 	return err;
 }

This patch is causing rtnl recursion and deadlocks on my system.

A daemon running on my system (NetworkManager) tries calling various wext ioctls on newly registered network interfaces, including 802.15.4 interfaces, which are dispatched from dev_ioctl() in net/core/dev_ioctl.c via wext_handle_ioctl() in net/wireless/wext-core.c to wireless_process_ioctl() to ioctl_standard_call() to ioctl_standard_iw_point() to mac802154_wpan_ioctl(), and wext_ioctl_dispatch() does:

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

Hi Lennert,

Quoted 44 lines

Quoted 35 lines

This patch fixes an issue to set address configuration with ioctl. Accessing the mib requires rtnl lock and the ndo_do_ioctl doesn't hold the rtnl lock while this callback is called. This patch do that manually.

Signed-off-by Alexander Aring <[email protected]>

Reported-by Matteo Petracca <[email protected]>

Reviewed-by Stefan Schmidt <[email protected]>

---
net/mac802154/iface.c | 5 +++--

1 file changed, 3 insertions(+), 2 deletions(-)

net/mac802154/iface.c +2 -2

index 91b75ab..2a58788 100644
@@ -62,8 +62,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		(struct sockaddr_ieee802154 *)&ifr->ifr_addr;
 	int err = -ENOIOCTLCMD;
 
-	ASSERT_RTNL();
-
+	rtnl_lock();
 	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {

@@ -90,6 +89,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
 			spin_unlock_bh(&sdata->mib_lock);
+			rtnl_unlock();
 			return -EBUSY;
 		}
 

net/mac802154/iface.c mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 117

Quoted 6 lines

net/mac802154/iface.c +1 -0

@@ -112,6 +112,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 
 	spin_unlock_bh(&sdata->mib_lock);
+	rtnl_unlock();
 	return err;
 }

This patch is causing rtnl recursion and deadlocks on my system.

Quoted 7 lines

A daemon running on my system (NetworkManager) tries calling various wext ioctls on newly registered network interfaces, including 802.15.4 interfaces, which are dispatched from dev_ioctl() in net/core/dev_ioctl.c via wext_handle_ioctl() in net/wireless/wext-core.c to wireless_process_ioctl() to ioctl_standard_call() to ioctl_standard_iw_point() to mac802154_wpan_ioctl(), and wext_ioctl_dispatch() does:

I would really wish that NM stops this insanity with poking at all network interfaces. No wonder that people just end up removing it. First it tries to poke nl80211 and then complaints why modules are auto-loaded even without WiFi cards present and now it tries to poke at WEXT for non-WiFi interfaces.

Quoted 18 lines

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

Quoted 7 lines

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

I think the ioctl handler should just first check which are its valid ioctl and also enforce permission in that run without the RTNL lock held. Return -EINVAL or -ENOIOCTLCMD when it gets a cmd that this function does not handle. And then have the function actually take the RTNL lock and process its two ioctl that it supports.

That way we should avoid the nasty with taking the RTNL lock for unsupported ioctl.

And hopefully at some point we can actually remove WEXT from the kernel. That it creates this nasty mess for other protocols is just insane.

Quoted 1 line · Marcel Holtmann

Hi Lennert,

Hi Marcel,

Quoted 52 lines

Quoted 44 lines

Quoted 35 lines

This patch fixes an issue to set address configuration with ioctl. Accessing the mib requires rtnl lock and the ndo_do_ioctl doesn't hold the rtnl lock while this callback is called. This patch do that manually.

Signed-off-by Alexander Aring <[email protected]>

Reported-by Matteo Petracca <[email protected]>

Reviewed-by Stefan Schmidt <[email protected]>

---
net/mac802154/iface.c | 5 +++--

1 file changed, 3 insertions(+), 2 deletions(-)

net/mac802154/iface.c +2 -2

index 91b75ab..2a58788 100644
@@ -62,8 +62,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		(struct sockaddr_ieee802154 *)&ifr->ifr_addr;
 	int err = -ENOIOCTLCMD;
 
-	ASSERT_RTNL();
-
+	rtnl_lock();
 	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {

@@ -90,6 +89,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
 			spin_unlock_bh(&sdata->mib_lock);
+			rtnl_unlock();
 			return -EBUSY;
 		}
 

net/mac802154/iface.c mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 117

Quoted 6 lines

net/mac802154/iface.c +1 -0

@@ -112,6 +112,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 
 	spin_unlock_bh(&sdata->mib_lock);
+	rtnl_unlock();
 	return err;
 }

This patch is causing rtnl recursion and deadlocks on my system.

Quoted 7 lines

A daemon running on my system (NetworkManager) tries calling various wext ioctls on newly registered network interfaces, including 802.15.4 interfaces, which are dispatched from dev_ioctl() in net/core/dev_ioctl.c via wext_handle_ioctl() in net/wireless/wext-core.c to wireless_process_ioctl() to ioctl_standard_call() to ioctl_standard_iw_point() to mac802154_wpan_ioctl(), and wext_ioctl_dispatch() does:

Quoted 5 lines

I would really wish that NM stops this insanity with poking at all network interfaces. No wonder that people just end up removing it. First it tries to poke nl80211 and then complaints why modules are auto-loaded even without WiFi cards present and now it tries to poke at WEXT for non-WiFi interfaces.

I can't say that I'm a NetworkManager fan myself... well, at least it's helping to make the kernel more robust in this case, I suppose. :(

Quoted 32 lines

Quoted 18 lines

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

Quoted 7 lines

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

I think the ioctl handler should just first check which are its valid ioctl and also enforce permission in that run without the RTNL lock held. Return -EINVAL or -ENOIOCTLCMD when it gets a cmd that this function does not handle. And then have the function actually take the RTNL lock and process its two ioctl that it supports.

Quoted 2 lines

That way we should avoid the nasty with taking the RTNL lock for unsupported ioctl.

Something like the below? (Alexander?) (I verified that it fixes the problem for me.)

As to adding permission checks to the 802.15.4 subsystem, that's a whole 'nother undertaking -- for one, I can currently snoop on any 802.15.4 traffic going through the box (including 6LoWPAN traffic) as an unprivileged user, which is somewhat crazy -- but that's probably something for another email (thread).

Quoted 4 lines

And hopefully at some point we can actually remove WEXT from the kernel. That it creates this nasty mess for other protocols is just insane.

I totally agree that the rtnl lock being held or not being held upon entering an ndo ioctl depending on what the ioctl cmd is is very bad, as this is just conditional locking which is bad in itself because of multiple reasons, and WEXT should be taken out and shot for causing this situation to be able to arise in the first place.

commit f5f4876bee64af07f6733d6bdb87ecb7e6fac85d

Author: Lennert Buytenhek <[email protected]>
Date:   Tue May 26 08:53:33 2015 +0300

mac802154: Avoid rtnl deadlock in mac802154_wpan_ioctl().

->ndo_do_ioctl() can be entered with the rtnl lock already held, for example when sending a wext ioctl to a device (in which case the rtnl lock is taken by wext_ioctl_dispatch()), but mac802154_wpan_ioctl() currently unconditionally takes the rtnl lock on entry, which can cause deadlocks.

To fix this, bail out of mac802154_wpan_ioctl() before taking the rtnl lock if the ioctl cmd is not one of the cmds we implement.

Signed-off-by Lennert Buytenhek <[email protected]>

net/mac802154/iface.c +0 -0

index f30788d..b544b5d 100644

Hi,

Quoted 101 lines · Lennert Buytenhek

Quoted 1 line · Marcel Holtmann

Hi Lennert,

Hi Marcel,

Quoted 52 lines

Quoted 44 lines

Quoted 35 lines

This patch fixes an issue to set address configuration with ioctl. Accessing the mib requires rtnl lock and the ndo_do_ioctl doesn't hold the rtnl lock while this callback is called. This patch do that manually.

Signed-off-by Alexander Aring <[email protected]>

Reported-by Matteo Petracca <[email protected]>

Reviewed-by Stefan Schmidt <[email protected]>

---
net/mac802154/iface.c | 5 +++--

1 file changed, 3 insertions(+), 2 deletions(-)

net/mac802154/iface.c +2 -2

index 91b75ab..2a58788 100644
@@ -62,8 +62,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 		(struct sockaddr_ieee802154 *)&ifr->ifr_addr;
 	int err = -ENOIOCTLCMD;
 
-	ASSERT_RTNL();
-
+	rtnl_lock();
 	spin_lock_bh(&sdata->mib_lock);
 
 	switch (cmd) {

@@ -90,6 +89,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case SIOCSIFADDR:
 		if (netif_running(dev)) {
 			spin_unlock_bh(&sdata->mib_lock);
+			rtnl_unlock();
 			return -EBUSY;
 		}
 

net/mac802154/iface.c mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 117

Quoted 6 lines

net/mac802154/iface.c +1 -0

@@ -112,6 +112,7 @@ mac802154_wpan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 
 	spin_unlock_bh(&sdata->mib_lock);
+	rtnl_unlock();
 	return err;
 }

This patch is causing rtnl recursion and deadlocks on my system.

Quoted 7 lines

A daemon running on my system (NetworkManager) tries calling various wext ioctls on newly registered network interfaces, including 802.15.4 interfaces, which are dispatched from dev_ioctl() in net/core/dev_ioctl.c via wext_handle_ioctl() in net/wireless/wext-core.c to wireless_process_ioctl() to ioctl_standard_call() to ioctl_standard_iw_point() to mac802154_wpan_ioctl(), and wext_ioctl_dispatch() does:

Quoted 5 lines

I would really wish that NM stops this insanity with poking at all network interfaces. No wonder that people just end up removing it. First it tries to poke nl80211 and then complaints why modules are auto-loaded even without WiFi cards present and now it tries to poke at WEXT for non-WiFi interfaces.

I can't say that I'm a NetworkManager fan myself... well, at least it's helping to make the kernel more robust in this case, I suppose. :(

Quoted 32 lines

Quoted 18 lines

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

Quoted 7 lines

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

I think the ioctl handler should just first check which are its valid ioctl and also enforce permission in that run without the RTNL lock held. Return -EINVAL or -ENOIOCTLCMD when it gets a cmd that this function does not handle. And then have the function actually take the RTNL lock and process its two ioctl that it supports.

Quoted 6 lines

Quoted 2 lines

That way we should avoid the nasty with taking the RTNL lock for unsupported ioctl.

Something like the below? (Alexander?) (I verified that it fixes the problem for me.)

I can suggest also other solutions which are:

- Remove the ndo_do_ioctl callback? If somebody wants to set address things, they should use netlink. There is also some warn notice in the callback "Using DEBUGing ioctl SIOCSIFADDR isn't recommended!\n", which reports the user that he is not doing the right thing when setting addresses via ioctl. That's very trustful.

I know currently there is some function "mac802154_wpan_update_llsec" which makes the security layer to work, because it's not called when setting short/panid anywhere else. I would be happy if we can "deactivate" the security layer -> removing old netlink interface and then try to making an easy to use interface over nl802154 to access the mib security entries. (I have started somewhere, but this is also another thread to talk about access security attributes over nl802154.

- Hold the rtnl lock only when the socket code is called. I insert this code because somebody tried to ioctl a 802.15.4 socket and the lock wasn't held. Maybe we can hold the rtnl lock in the caller, see [0].

Quoted 56 lines

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

Quoted 7 lines

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

I think the ioctl handler should just first check which are its valid ioctl and also enforce permission in that run without the RTNL lock held. Return -EINVAL or -ENOIOCTLCMD when it gets a cmd that this function does not handle. And then have the function actually take the RTNL lock and process its two ioctl that it supports.

Quoted 6 lines

Quoted 2 lines

That way we should avoid the nasty with taking the RTNL lock for unsupported ioctl.

Something like the below? (Alexander?) (I verified that it fixes the problem for me.)

I can suggest also other solutions which are:

- Remove the ndo_do_ioctl callback? If somebody wants to set address things, they should use netlink. There is also some warn notice in the callback "Using DEBUGing ioctl SIOCSIFADDR isn't recommended!\n", which reports the user that he is not doing the right thing when setting addresses via ioctl. That's very trustful.

I know currently there is some function "mac802154_wpan_update_llsec" which makes the security layer to work, because it's not called when setting short/panid anywhere else. I would be happy if we can "deactivate" the security layer -> removing old netlink interface and then try to making an easy to use interface over nl802154 to access the mib security entries. (I have started somewhere, but this is also another thread to talk about access security attributes over nl802154.

Quoted 3 lines

- Hold the rtnl lock only when the socket code is called. I insert this code because somebody tried to ioctl a 802.15.4 socket and the lock wasn't held. Maybe we can hold the rtnl lock in the caller, see [0].

besides I don't know why they use a check on ARPHRD there, a 802.15.4 socket can also be created on wpan intefaces only, which should be checked at creating the socket or somewhere.

A general question and sorry for this unknowledge is that: is it normal than a socket ioctl will call the netdev ioctl?

Quoted 56 lines · Alexander Aring

Quoted 56 lines

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

Quoted 7 lines

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

I think the ioctl handler should just first check which are its valid ioctl and also enforce permission in that run without the RTNL lock held. Return -EINVAL or -ENOIOCTLCMD when it gets a cmd that this function does not handle. And then have the function actually take the RTNL lock and process its two ioctl that it supports.

Quoted 6 lines

Quoted 2 lines

That way we should avoid the nasty with taking the RTNL lock for unsupported ioctl.

Something like the below? (Alexander?) (I verified that it fixes the problem for me.)

I can suggest also other solutions which are:

- Remove the ndo_do_ioctl callback? If somebody wants to set address things, they should use netlink. There is also some warn notice in the callback "Using DEBUGing ioctl SIOCSIFADDR isn't recommended!\n", which reports the user that he is not doing the right thing when setting addresses via ioctl. That's very trustful.

I know currently there is some function "mac802154_wpan_update_llsec" which makes the security layer to work, because it's not called when setting short/panid anywhere else. I would be happy if we can "deactivate" the security layer -> removing old netlink interface and then try to making an easy to use interface over nl802154 to access the mib security entries. (I have started somewhere, but this is also another thread to talk about access security attributes over nl802154.

Quoted 7 lines

Quoted 3 lines

- Hold the rtnl lock only when the socket code is called. I insert this code because somebody tried to ioctl a 802.15.4 socket and the lock wasn't held. Maybe we can hold the rtnl lock in the caller, see [0].

besides I don't know why they use a check on ARPHRD there, a 802.15.4 socket can also be created on wpan intefaces only, which should be checked at creating the socket or somewhere.

If you're talking about this code in net/ieee802154/socket.c:

===
static int ieee802154_dev_ioctl(struct sock *sk, struct ifreq __user *arg,
unsigned int cmd)

{
        struct ifreq ifr;
        int ret = -ENOIOCTLCMD;
        struct net_device *dev;

        if (copy_from_user(&ifr, arg, sizeof(struct ifreq)))
                return -EFAULT;

        ifr.ifr_name[IFNAMSIZ-1] = 0;

        dev_load(sock_net(sk), ifr.ifr_name);
        dev = dev_get_by_name(sock_net(sk), ifr.ifr_name);

        if (!dev)
                return -ENODEV;

        if (dev->type == ARPHRD_IEEE802154 && dev->netdev_ops->ndo_do_ioctl)
                ret = dev->netdev_ops->ndo_do_ioctl(dev, &ifr, cmd);

        if (!ret && copy_to_user(arg, &ifr, sizeof(struct ifreq)))
                ret = -EFAULT;
        dev_put(dev);

        return ret;
}

static int ieee802154_sock_ioctl(struct socket *sock, unsigned int cmd,
unsigned long arg)

{
        struct sock *sk = sock->sk;

        switch (cmd) {
        case SIOCGSTAMP:
                return sock_get_timestamp(sk, (struct timeval __user *)arg);
        case SIOCGSTAMPNS:
                return sock_get_timestampns(sk, (struct timespec __user *)arg);
        case SIOCGIFADDR:
        case SIOCSIFADDR:
                return ieee802154_dev_ioctl(sk, (struct ifreq __user *)arg,
                                cmd);
        default:
                if (!sk->sk_prot->ioctl)
                        return -ENOIOCTLCMD;
                return sk->sk_prot->ioctl(sk, cmd, arg);
        }
}

===

The network namespace is taken from the 802.15.4 socket, but the device name is taken from the struct ifreq that's passed in, which does not necessarily refer to a 802.15.4 device.

Quoted 3 lines

A general question and sorry for this unknowledge is that: is it normal than a socket ioctl will call the netdev ioctl?

netdevice(7) has a note about this. I think it's probably just because of historical reasons, because there is no 'dedicated' way of calling a netdev ioctl other than going through the socket (which, historically, means PF_INET) layer. Not very clean at all, but this is unlikely to ever change at this point. :-/

Hi,

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

static int wext_ioctl_dispatch(struct net *net, struct ifreq *ifr,
unsigned int cmd, struct iw_request_info *info, wext_ioctl_func standard, wext_ioctl_func private)

{
       int ret = wext_permission_check(cmd);

       if (ret)
               return ret;

       dev_load(net, ifr->ifr_name);
       rtnl_lock();
       ret = wireless_process_ioctl(net, ifr, cmd, info, standard, private);
       rtnl_unlock();

       return ret;
}

Quoted 7 lines

leading to a double rtnl lock in mac802154_wpan_ioctl().

The easy fix would be to have mac802154_wpan_ioctl() check the ioctl cmd before taking the rtnl -- I'm not too happy about the rtnl being held or not held upon entry to a device ioctl depending on the ioctl cmd and/or on who the caller is, but we might not be easily able to change this...

I think the ioctl handler should just first check which are its valid ioctl and also enforce permission in that run without the RTNL lock held. Return -EINVAL or -ENOIOCTLCMD when it gets a cmd that this function does not handle. And then have the function actually take the RTNL lock and process its two ioctl that it supports.

Quoted 5 lines

Quoted 2 lines

That way we should avoid the nasty with taking the RTNL lock for unsupported ioctl.

Something like the below? (Alexander?) (I verified that it fixes the problem for me.)

I can suggest also other solutions which are:

- Remove the ndo_do_ioctl callback? If somebody wants to set address things, they should use netlink. There is also some warn notice in the callback "Using DEBUGing ioctl SIOCSIFADDR isn't recommended!\n", which reports the user that he is not doing the right thing when setting addresses via ioctl. That's very trustful.

I know currently there is some function "mac802154_wpan_update_llsec" which makes the security layer to work, because it's not called when setting short/panid anywhere else. I would be happy if we can "deactivate" the security layer -> removing old netlink interface and then try to making an easy to use interface over nl802154 to access the mib security entries. (I have started somewhere, but this is also another thread to talk about access security attributes over nl802154.

- Hold the rtnl lock only when the socket code is called. I insert this code because somebody tried to ioctl a 802.15.4 socket and the lock wasn't held. Maybe we can hold the rtnl lock in the caller, see [0]. But this requires that all other callers hold the rtnl lock while calling this callback and I don't know if this is true.

Nevertheless your solutions will reduce to hold the rtnl_lock if not necessary and fixes the nm issue. So you get a:

Acked-by Alexander Aring <[email protected]>

If you see that to remove the ioctl it's a good idea (okay that's maybe not a good idea because there are at least one user outside which use this feature) you can send a patch for it.

Quoted 5 lines

- Hold the rtnl lock only when the socket code is called. I insert this code because somebody tried to ioctl a 802.15.4 socket and the lock wasn't held. Maybe we can hold the rtnl lock in the caller, see [0]. But this requires that all other callers hold the rtnl lock while calling this callback and I don't know if this is true.

I think that it makes sense to keep the invariant that upon entry of an ioctl, no locks are guaranteed to be held. WEXT violates this principle, but I would say that the WEXT behavior is undesirable, as it's not a good thing to take a bunch of locks just in case a callee might need them, and it will cause code to be written that will (silently) depend on these assumptions and it will be hard to untangle those dependencies later on, or even impossible to do in case it becomes part of an API/ABI (like it is now in the WEXT case).

Quoted 5 lines

Nevertheless your solutions will reduce to hold the rtnl_lock if not necessary and fixes the nm issue. So you get a:

Acked-by Alexander Aring <[email protected]>

Cool, I'll do this and submit it.

Quoted 4 lines

If you see that to remove the ioctl it's a good idea (okay that's maybe not a good idea because there are at least one user outside which use this feature) you can send a patch for it.

I agree that this ioctl is an ugly wart, and that it should probably be scheduled to be removed entirely. Do you have visibility into what userspace code might be using this -- I know that there's a zigbee stack, but I don't think we can see it?

this patch series contains patches for remove pib and mib lock. We have now a other locking strategie inside this stack which requires at the moment the rtnl lock only. The first patch will hold the rtnl lock which is required to access the mib attributes while calling socket ioctl. The last patch will use atomic operations for incremet the dsn value for dataframes, currently it could be that this value is the same in two contiguous frames which could be occur a wrong ack frame handling in ARET mode.

- Alex

changes since v2:
- switch patch 3 and 4.

Alexander Aring (4):
mac802154: fix hold rtnl while ioctl
mac802154: remove pib lock
mac802154: use atomic ops for sequence incrementation
mac802154: remove mib lock

include/net/cfg802154.h         |  6 ++--
include/net/ieee802154_netdev.h | 10 -------
net/ieee802154/6lowpan/core.c   | 28 ------------------
net/ieee802154/6lowpan/tx.c     |  2 +-
net/ieee802154/core.c           |  2 --
net/ieee802154/nl-mac.c         | 14 ++++++---
net/ieee802154/nl-phy.c         |  6 ++--
net/ieee802154/socket.c         | 12 ++++----
net/mac802154/ieee802154_i.h    |  7 -----
net/mac802154/iface.c           | 29 ++++++-------------
net/mac802154/mac_cmd.c         |  7 ++---
net/mac802154/mib.c             | 63 ++---------------------------------------
net/mac802154/rx.c              |  5 ----
13 files changed, 34 insertions(+), 157 deletions(-)