On Tue, 1 Oct 2019 at 16:39, Johannes Berg [off-list ref] wrote:

Hi,

On Sun, 2019-09-29 at 17:31 +0900, Taehee Yoo wrote:

quoted

virt_wifi case is a little bit different case.

Well, arguably, it was also just missing this - it just looks different
:)

quoted

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.

quoted

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!

quoted

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.

quoted

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