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
