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

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

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