Message ID | 20190928164843.31800-1-ap420073@gmail.com |
---|---|
Headers | show |
Series | net: fix nested device bugs | expand |
> VLAN, BONDING, TEAM, MACSEC, MACVLAN, IPVLAN, VIRT_WIFI and VXLAN. > But I couldn't test all interface types so there could be more device > types which have similar problems. Did you test virt_wifi? I don't see how it *doesn't* have the nesting problem, and you didn't change it? No, I see. You're limiting the nesting generally now in patch 1, and the others are just lockdep fixups (I guess it's surprising virt_wifi doesn't do this at all?). FWIW I don't think virt_wifi really benefits at all from stacking, so we could just do something like --- a/drivers/net/wireless/virt_wifi.c +++ b/drivers/net/wireless/virt_wifi.c @@ -508,6 +508,9 @@ static int virt_wifi_newlink(struct net *src_net, struct net_device *dev, else if (dev->mtu > priv->lowerdev->mtu) return -EINVAL; + if (priv->lowerdev->ieee80211_ptr) + return -EINVAL; + err = netdev_rx_handler_register(priv->lowerdev, virt_wifi_rx_handler, priv); if (err) { IMHO, but of course generally limiting the stack depth is needed anyway and solves the problem well enough for virt_wifi. johannes
On Sun, 29 Sep 2019 at 04:20, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > VLAN, BONDING, TEAM, MACSEC, MACVLAN, IPVLAN, VIRT_WIFI and VXLAN. > > But I couldn't test all interface types so there could be more device > > types which have similar problems. > > Did you test virt_wifi? I don't see how it *doesn't* have the nesting > problem, and you didn't change it? > > No, I see. You're limiting the nesting generally now in patch 1, and the > others are just lockdep fixups (I guess it's surprising virt_wifi > doesn't do this at all?). virt_wifi case is a little bit different case. I add the last patch that is to fix refcnt leaks in the virt_wifi module. The way to fix this is to add notifier routine. The notifier routine could delete lower device before deleting virt_wifi device. If virt_wifi devices are nested, notifier would work recursively. At that time, it would make stack memory overflow. Actually, before this patch, virt_wifi doesn't have the same problem. So, I will update a comment in a v5 patch. > > FWIW I don't think virt_wifi really benefits at all from stacking, so we > could just do something like > > --- a/drivers/net/wireless/virt_wifi.c > +++ b/drivers/net/wireless/virt_wifi.c > @@ -508,6 +508,9 @@ static int virt_wifi_newlink(struct net *src_net, struct net_device *dev, > else if (dev->mtu > priv->lowerdev->mtu) > return -EINVAL; > > + if (priv->lowerdev->ieee80211_ptr) > + return -EINVAL; > + > err = netdev_rx_handler_register(priv->lowerdev, virt_wifi_rx_handler, > priv); > if (err) { > Many other devices use this way to avoid wrong nesting configuration. And I think it's a good way. But we should think about the below configuration. vlan5 | virt_wifi4 | vlan3 | virt_wifi2 | vlan1 | dummy0 That code wouldn't avoid this configuration. And all devices couldn't avoid this config. I have been considering this case, but I couldn't make a decision yet. Maybe common netdev function is needed to find the same device type in their graph. > > > IMHO, but of course generally limiting the stack depth is needed anyway > and solves the problem well enough for virt_wifi. > > This is a little bit different question for you. I found another bug in virt_wifi after my last patch. Please test below commands ip link add dummy0 type dummy ip link add vw1 link dummy0 type virt_wifi ip link add vw2 link vw1 type virt_wifi modprobe -rv virt_wifi Then, you can see the warning messages. If SET_NETDEV_DEV() is deleted in the virt_wifi_newlink(), you can avoid that warning message. But I'm not sure about it's safe to remove that. I would really appreciate it if you let me know about that. > johannes >
On Sun, 2019-09-29 at 17:31 +0900, Taehee Yoo wrote: > virt_wifi case is a little bit different case. Well, arguably, it was also just missing this - it just looks different :) > I add the last patch that is to fix refcnt leaks in the virt_wifi module. > The way to fix this is to add notifier routine. > The notifier routine could delete lower device before deleting > virt_wifi device. > If virt_wifi devices are nested, notifier would work recursively. > At that time, it would make stack memory overflow. > > Actually, before this patch, virt_wifi doesn't have the same problem. > So, I will update a comment in a v5 patch. OK, sure. > Many other devices use this way to avoid wrong nesting configuration. > And I think it's a good way. > But we should think about the below configuration. > > vlan5 > | > virt_wifi4 > | > vlan3 > | > virt_wifi2 > | > vlan1 > | > dummy0 > > That code wouldn't avoid this configuration. > And all devices couldn't avoid this config. Good point, so then really that isn't useful to check - most people won't try to set it up that way (since it's completely useless) and if they do anyway too much nesting would be caught by your patchset here. > I have been considering this case, but I couldn't make a decision yet. > Maybe common netdev function is needed to find the same device type > in their graph. I don't think it's worthwhile just to prevent somebody from making a configuration that we think now is nonsense. Perhaps they do have some kind of useful use-case for it ... > This is a little bit different question for you. > I found another bug in virt_wifi after my last patch. > Please test below commands > ip link add dummy0 type dummy > ip link add vw1 link dummy0 type virt_wifi > ip link add vw2 link vw1 type virt_wifi > modprobe -rv virt_wifi > > Then, you can see the warning messages. > If SET_NETDEV_DEV() is deleted in the virt_wifi_newlink(), > you can avoid that warning message. > But I'm not sure about it's safe to remove that. > I would really appreciate it if you let me know about that. Hmm, I don't see any warnings. SET_NETDEV_DEV() should be there though. Do you see the same if you stack it with something else inbetween? If not, I guess preventing virt_wifi from stacking on top of itself would be sufficient ... johannes
On Tue, 1 Oct 2019 at 16:39, Johannes Berg <johannes@sipsolutions.net> wrote: > Hi, > On Sun, 2019-09-29 at 17:31 +0900, Taehee Yoo wrote: > > > virt_wifi case is a little bit different case. > > Well, arguably, it was also just missing this - it just looks different > :) > > > I add the last patch that is to fix refcnt leaks in the virt_wifi module. > > The way to fix this is to add notifier routine. > > The notifier routine could delete lower device before deleting > > virt_wifi device. > > If virt_wifi devices are nested, notifier would work recursively. > > At that time, it would make stack memory overflow. > > > > Actually, before this patch, virt_wifi doesn't have the same problem. > > So, I will update a comment in a v5 patch. > > OK, sure. > > > Many other devices use this way to avoid wrong nesting configuration. > > And I think it's a good way. > > But we should think about the below configuration. > > > > vlan5 > > | > > virt_wifi4 > > | > > vlan3 > > | > > virt_wifi2 > > | > > vlan1 > > | > > dummy0 > > > > That code wouldn't avoid this configuration. > > And all devices couldn't avoid this config. > > Good point, so then really that isn't useful to check - most people > won't try to set it up that way (since it's completely useless) and if > they do anyway too much nesting would be caught by your patchset here. > Yes, Thanks! > > I have been considering this case, but I couldn't make a decision yet. > > Maybe common netdev function is needed to find the same device type > > in their graph. > > I don't think it's worthwhile just to prevent somebody from making a > configuration that we think now is nonsense. Perhaps they do have some > kind of useful use-case for it ... > I agree with your opinion. > > This is a little bit different question for you. > > I found another bug in virt_wifi after my last patch. > > Please test below commands > > ip link add dummy0 type dummy > > ip link add vw1 link dummy0 type virt_wifi > > ip link add vw2 link vw1 type virt_wifi > > modprobe -rv virt_wifi > > > > Then, you can see the warning messages. > > If SET_NETDEV_DEV() is deleted in the virt_wifi_newlink(), > > you can avoid that warning message. > > But I'm not sure about it's safe to remove that. > > I would really appreciate it if you let me know about that. > > Hmm, I don't see any warnings. SET_NETDEV_DEV() should be there though. Okay, thanks. I will do not remove SET_NETDEV_DEV() in a v5 patch. > Do you see the same if you stack it with something else inbetween? If > not, I guess preventing virt_wifi from stacking on top of itself would > be sufficient ... > Yes, the below test commands will make warning messages. So, I will add a new patch for this without removing SET_NETDEV_DEV(). Reproducer : ip link add dummy0 type dummy ip link add vw1 link dummy0 type virt_wifi ip link add vlan2 link vw1 type vlan id 1 ip link add vw3 link vlan2 type virt_wifi modprobe -rv virt_wifi Messages: [12734.236946] sysfs group 'byte_queue_limits' not found for kobject 'tx-0' [12734.238862] WARNING: CPU: 1 PID: 19710 at fs/sysfs/group.c:280 sysfs_remove_group+0x11b/0x170 [ ... ] 12734.256132] Call Trace: [12734.256430] netdev_queue_update_kobjects+0x1f5/0x340 [12734.257025] netdev_unregister_kobject+0x142/0x1d0 [12734.257580] rollback_registered_many+0x618/0xc80 [12734.258175] ? notifier_call_chain+0x90/0x160 [12734.258688] ? generic_xdp_install+0x310/0x310 [12734.259208] ? netdev_upper_dev_unlink+0x114/0x180 [12734.259791] unregister_netdevice_many.part.126+0x13/0x1b0 [12734.260434] __rtnl_link_unregister+0x156/0x320 [12734.260967] ? rtnl_unregister_all+0x120/0x120 [ ... ] [12734.283395] sysfs group 'power' not found for kobject 'vw3' [12734.284081] WARNING: CPU: 1 PID: 19710 at fs/sysfs/group.c:280 sysfs_remove_group+0x11b/0x170 [ ... ] [12734.337509] sysfs group 'statistics' not found for kobject 'vw3' [12734.338375] WARNING: CPU: 1 PID: 19710 at fs/sysfs/group.c:280 sysfs_remove_group+0x11b/0x170 [ ... ] [12734.391687] sysfs group 'wireless' not found for kobject 'vw3' [12734.392525] WARNING: CPU: 1 PID: 19710 at fs/sysfs/group.c:280 sysfs_remove_group+0x11b/0x170 [ ... ] > johannes > Thanks, Taehee Yoo