[PATCH net] bridge: check iface upper dev when setting master via ioctl

[PATCH net] bridge: check iface upper dev when setting master via ioctl

[Hangbin Liu]

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) */
-- 
2.5.5

Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl

[Nikolay Aleksandrov]

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.

Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl

[Nikolay Aleksandrov]

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

Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl

[Hangbin Liu]

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

Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl

[Nikolay Aleksandrov]

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
> 

[PATCHv2 net] bridge: check iface upper dev when setting master via ioctl

[Hangbin Liu]

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. It would be better to return as early
as possible.

v1 -> v2:
use netdev_master_upper_dev_get() instead of netdev_has_any_upper_dev()
to check if we have a master, because not all upper devs are masters,
e.g. vlan device.

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..5bb6681 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_master_upper_dev_get(dev))
 		return -EBUSY;
 
 	/* No bridging devices that dislike that (e.g. wireless) */
-- 
2.5.5

Re: [PATCHv2 net] bridge: check iface upper dev when setting master via ioctl

[Nikolay Aleksandrov]

On 27/04/18 15:59, 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. It would be better to return as early
> as possible.
> 
> v1 -> v2:
> use netdev_master_upper_dev_get() instead of netdev_has_any_upper_dev()
> to check if we have a master, because not all upper devs are masters,
> e.g. vlan device.
> 
> 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..5bb6681 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_master_upper_dev_get(dev))
>   		return -EBUSY;
>   
>   	/* No bridging devices that dislike that (e.g. wireless) */
> 

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCHv2 net] bridge: check iface upper dev when setting master via ioctl

[David Miller]

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Fri, 27 Apr 2018 20:59:24 +0800

> 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. It would be better to return as early
> as possible.
> 
> v1 -> v2:
> use netdev_master_upper_dev_get() instead of netdev_has_any_upper_dev()
> to check if we have a master, because not all upper devs are masters,
> e.g. vlan device.
> 
> Reported-by: syzbot+de73361ee4971b6e6f75@syzkaller.appspotmail.com
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied and queued up for -stable, thanks.