| Message ID | 1524750986-23904-1-git-send-email-liuhangbin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] bridge: check iface upper dev when setting master via ioctl | expand |
On 26/04/18 16:56, Hangbin Liu wrote: > When we set a bond slave's master to bridge via ioctl, we only check > the IFF_BRIDGE_PORT flag. Although we will find the slave's real master > at netdev_master_upper_dev_link() later, it already does some settings > and allocates some resources. So it would be better to return as early > as possible. > > Reported-by: syzbot+de73361ee4971b6e6f75@syzkaller.appspotmail.com > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > net/bridge/br_if.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index 82c1a6f..176de8a9 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -518,8 +518,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev, > return -ELOOP; > } > > - /* Device is already being bridged */ > - if (br_port_exists(dev)) > + /* Device has master upper dev */ > + if (netdev_has_any_upper_dev(dev)) > return -EBUSY; > > /* No bridging devices that dislike that (e.g. wireless) */ > Not all upper devs are masters. This can break some setups.
On 26/04/18 17:00, Nikolay Aleksandrov wrote: > On 26/04/18 16:56, Hangbin Liu wrote: >> When we set a bond slave's master to bridge via ioctl, we only check >> the IFF_BRIDGE_PORT flag. Although we will find the slave's real master >> at netdev_master_upper_dev_link() later, it already does some settings >> and allocates some resources. So it would be better to return as early >> as possible. >> >> Reported-by: syzbot+de73361ee4971b6e6f75@syzkaller.appspotmail.com >> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> >> --- >> net/bridge/br_if.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index 82c1a6f..176de8a9 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -518,8 +518,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev, >> return -ELOOP; >> } >> >> - /* Device is already being bridged */ >> - if (br_port_exists(dev)) >> + /* Device has master upper dev */ >> + if (netdev_has_any_upper_dev(dev)) >> return -EBUSY; >> >> /* No bridging devices that dislike that (e.g. wireless) */ >> > > Not all upper devs are masters. This can break some setups. > > 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. 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 needed even there, but it doesn't hurt either so up to you. Thanks, Nik
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; > > > > > > 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. > > 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? [1] https://syzkaller.appspot.com/bug?id=3e0339080acd6a2a350a900bc6533b03f5498490 Thanks Hangbin
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 >
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index 82c1a6f..176de8a9 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -518,8 +518,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev, return -ELOOP; } - /* Device is already being bridged */ - if (br_port_exists(dev)) + /* Device has master upper dev */ + if (netdev_has_any_upper_dev(dev)) return -EBUSY; /* No bridging devices that dislike that (e.g. wireless) */
When we set a bond slave's master to bridge via ioctl, we only check the IFF_BRIDGE_PORT flag. Although we will find the slave's real master at netdev_master_upper_dev_link() later, it already does some settings and allocates some resources. So it would be better to return as early as possible. Reported-by: syzbot+de73361ee4971b6e6f75@syzkaller.appspotmail.com Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- net/bridge/br_if.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)