From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl Date: Thu, 26 Apr 2018 17:22:46 +0300 Message-ID: References: <1524750986-23904-1-git-send-email-liuhangbin@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Dmitry Vyukov , syzbot , David Miller To: Hangbin Liu , netdev@vger.kernel.org Return-path: Received: from mail-wr0-f195.google.com ([209.85.128.195]:37130 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756111AbeDZOWu (ORCPT ); Thu, 26 Apr 2018 10:22:50 -0400 Received: by mail-wr0-f195.google.com with SMTP id c14-v6so16859062wrd.4 for ; Thu, 26 Apr 2018 07:22:50 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> 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