From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hangbin Liu Subject: Re: [PATCH net] bridge: check iface upper dev when setting master via ioctl Date: Fri, 27 Apr 2018 09:31:02 +0800 Message-ID: <20180427013102.GJ20683@leo.usersys.redhat.com> References: <1524750986-23904-1-git-send-email-liuhangbin@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, Dmitry Vyukov , syzbot , David Miller To: Nikolay Aleksandrov Return-path: Received: from mail-pf0-f179.google.com ([209.85.192.179]:42810 "EHLO mail-pf0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751279AbeD0BbN (ORCPT ); Thu, 26 Apr 2018 21:31:13 -0400 Received: by mail-pf0-f179.google.com with SMTP id a11so226830pfn.9 for ; Thu, 26 Apr 2018 18:31:13 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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