Re: [PATCHv2 bluetooth-next 1/4] mac802154: fix hold rtnl while ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lennert,

>> 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 <alex.aring@xxxxxxxxx>
>> Reported-by: Matteo Petracca <matteo.petracca@xxxxxxxx>
>> Reviewed-by: Stefan Schmidt <stefan@xxxxxxxxxxxxxxx>
>> ---
>> net/mac802154/iface.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
>> index 91b75ab..2a58788 100644
>> --- a/net/mac802154/iface.c
>> +++ b/net/mac802154/iface.c
>> @@ -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;
>> 		}
>> 
>> @@ -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: 

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.

> 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...

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.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux