On 27/04/18 04:31, Hangbin Liu wrote:
Hi Nikolay,
Thanks for the comments.
On Thu, Apr 26, 2018 at 05:22:46PM +0300, Nikolay Aleksandrov wrote:
Not all upper devs are masters. This can break some setups.
Ah, like vlan device.. So how about
+ if (netdev_master_upper_dev_get(dev))
return -EBUSY;
That should be fine, yes.
Also it's not really a bug, the device begins to get initialized but it
will get removed at netdev_master_upper_dev_link() anyway if there's
already a master. Why would it be better ?
It's clearly wrong to try and enslave a device that already has a master
via ioctl, rtnetlink already deals with that and the old ioctl interface
will get an error, yes it will initialize some structs but they'll get
freed later. This is common practice, check the bonding for example.
Bonding use netdev_is_rx_handler_busy(slave_dev) to check if the slave
already has a master, which is another solution.
Some masters don't use rx_handlers and the bonding fails at linking them
as a master which is still fine, it cleans up after the error like the bridge.
If anything do the check in the ioctl interface (add_del_if) only and
maybe target net-next, there's really no bug fix here. IMO it's not
What if someone do like
while true; do brctl addif br0 bond_slave &; done
I know this is stupid and almost no one will do that in real world.
But syzbot run some similar test and get warn from kobject_add_internal()
with -ENOMEM. That's why I think we should fix it before allocate any
resource.
What do you think?
The bridge code is only a symptom of what happened, that warn was placed to
warn people against doing stupid things - it was literally in the commit message
of some kobject patch. As long as the resources involved are cleaned up and it's
returned to the bridge to cleanup after itself, it should be fine.
You can add the check if you feel like it, I don't have an
objection against failing earlier. My main concern was the netdev_has_any_upper
usage which can break some setups.
Cheers,
Nik
[1]
https://syzkaller.appspot.com/bug?id=3e0339080acd6a2a350a900bc6533b03f5498490
Thanks
Hangbin
