From mboxrd@z Thu Jan 1 00:00:00 1970 From: Toshiaki Makita Subject: Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb entries in br_fdb_changeaddr Date: Wed, 08 Jan 2014 15:02:36 +0900 Message-ID: <1389160956.3989.1.camel@ubuntu-vm-makita> References: <1387281821-21342-1-git-send-email-makita.toshiaki@lab.ntt.co.jp> <1387281821-21342-2-git-send-email-makita.toshiaki@lab.ntt.co.jp> <52C70F63.4020604@redhat.com> <52C7218C.2000205@redhat.com> <1388935578.1906.26.camel@localhost.localdomain> <52CA9398.3090603@redhat.com> <1389098578.3768.12.camel@ubuntu-vm-makita> <52CC12CA.7030001@redhat.com> <1389112385.1751.17.camel@localhost.localdomain> <52CC3D51.6090903@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Toshiaki Makita , "David S . Miller" , Stephen Hemminger , netdev@vger.kernel.org To: vyasevic@redhat.com Return-path: Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:45495 "EHLO tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752932AbaAHGDI (ORCPT ); Wed, 8 Jan 2014 01:03:08 -0500 In-Reply-To: <52CC3D51.6090903@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, 2014-01-07 at 12:45 -0500, Vlad Yasevich wrote: > On 01/07/2014 11:33 AM, Toshiaki Makita wrote: > > On Tue, 2014-01-07 at 09:44 -0500, Vlad Yasevich wrote: > >> On 01/07/2014 07:42 AM, Toshiaki Makita wrote: > >>> On Mon, 2014-01-06 at 06:29 -0500, Vlad Yasevich wrote: > >>>> On 01/05/2014 10:26 AM, Toshiaki Makita wrote: > >>>>> On Fri, 2014-01-03 at 15:46 -0500, Vlad Yasevich wrote: > >>>>>> On 01/03/2014 02:28 PM, Vlad Yasevich wrote: > >>>>>>> On 12/17/2013 07:03 AM, Toshiaki Makita wrote: > >>>>>>>> br_fdb_changeaddr() assumes that there is at most one local entry per port > >>>>>>>> per vlan. It used to be true, but since commit 36fd2b63e3b4 ("bridge: allow > >>>>>>>> creating/deleting fdb entries via netlink"), it has not been so. > >>>>>>>> Therefore, the function might fail to search a correct previous address > >>>>>>>> to be deleted and delete an arbitrary local entry if user has added local > >>>>>>>> entries manually. > >>>>>>>> > >>>>>>>> Example of problematic case: > >>>>>>>> ip link set eth0 address ee:ff:12:34:56:78 > >>>>>>>> brctl addif br0 eth0 > >>>>>>>> bridge fdb add 12:34:56:78:90:ab dev eth0 master > >>>>>>>> ip link set eth0 address aa:bb:cc:dd:ee:ff > >>>>>>>> Then, the address 12:34:56:78:90:ab might be deleted instead of > >>>>>>>> ee:ff:12:34:56:78, the original mac address of eth0. > >>>>>>>> > >>>>>>>> Address this issue by introducing a new flag, added_by_user, to struct > >>>>>>>> net_bridge_fdb_entry. > >>>>>>>> > >>>>>>>> Note that br_fdb_delete_by_port() has to set added_by_user to 0 in case > >>>>>>>> like: > >>>>>>>> ip link set eth0 address 12:34:56:78:90:ab > >>>>>>>> ip link set eth1 address aa:bb:cc:dd:ee:ff > >>>>>>>> brctl addif br0 eth0 > >>>>>>>> bridge fdb add aa:bb:cc:dd:ee:ff dev eth0 master > >>>>>>>> brctl addif br0 eth1 > >>>>>>>> brctl delif br0 eth0 > >>>>>>>> In this case, kernel should delete the user-added entry aa:bb:cc:dd:ee:ff, > >>>>>>>> but it also should have been added by "brctl addif br0 eth1" originally, > >>>>>>>> so we don't delete it and treat it a new kernel-created entry. > >>>>>>>> > >>>>>>> > >>>>>>> I was looking over my patch series that adds something similar to this > >>>>>>> and noticed that you are not handing the NTF_USE case. That case was > >>>>>>> always troublesome for me as it allows for 2 different way to create > >>>>>>> the same FDB: one through br_fdb_update() and one through fdb_add_entry(). > >>>>>>> > >>>>>>> It is possible, though I haven't found any users yet, that NTF_USE > >>>>>>> may be used and in that case, bridge will create a dynamic fdb and > >>>>>>> disregard all NUD flags. In case case, add_by_user will not be set > >>>>>>> either. > >>>>>>> > >>>>>>> I think that the above is broken and plan to submit a fix shortly. > >>>>>> > >>>>>> Just looked again at my NTF_USE patch and while it seems ok, the whole > >>>>>> NTF_USE usage is racy to begin with and I am really starting to question > >>>>>> it's validity. > >>>>>> > >>>>>> Presently, br_fdb_update() will not update local fdb entries. Instead > >>>>>> it will log a misleading warning... It will only let you update > >>>>>> non-local entries. This is fine for user-created entries, but any > >>>>>> operation on dynamically created entries will only persist until > >>>>>> the next packet. It also races against the packet, so there is > >>>>>> absolutely no guarantee that the values of fdb->dst and fdb->updated > >>>>>> will be consistent.. > >>>>>> > >>>>>> It seems to me that the update capability of NTF_USE would actually be > >>>>>> of more value on local or user-created fdb entries. > >>>>>> > >>>>>> The fdb creation capability of NTF_USE should be disabled. > >>>>>> > >>>>>> Thoughts? > >>>>> > >>>>> I ignored NTF_USE in this patch because I regard it as emulating kernel > >>>>> creating entries after investigating git log. > >>>>> > >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0c5c2d3089068d4aa378f7a40d2b5ad9d4f52ce8 > >>>>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=292d1398983f3514a0eab13b7606df7f4730b498 > >>>>> > >>>>> So I think NTF_USE shouldn't set added_by_user. > >>>>> And to emulate kernel creating entries, simply calling br_fdb_update() > >>>>> is the right way, isn't it? > >>>> > >>>> You can create dynamic entries (emulating the kernel) without NTF_USE. > >>>> Just set the NUD_REACHABLE. Notice that arp cache only uses NTF_USE > >>>> to trigger and arp notification. The creation is still triggered via > >>>> other netlink flags. > >>>> > >>>> The more I look at this the more I think NTF_USE should not create > >>>> an entry all by itself. > >>> > >>> I haven't fully understood you yet. > >>> Currently NTF_USE behaves as if the port receives a frame and it seems > >>> to work, though the ability to create entries is different from neigh > >>> subsystem. > >>> Why do you want to change the behavior? > >>> Are you worried about inconsistency of NLM-flags/NUD-state with NTF_USE > >>> between neigh and bridge? > >> > >> No, it is inconsistent with other NLM/NUD-state within bridge. As > >> an fdb creation flag NTF_USE is confusing. It will create an entry > >> without NLM_F_CREATE being set. It will ignore NLM_F_EXCL flag as > >> well. It will additionally ignore any NUD-state flags that may be set > >> in the netlink message. So it may not be doing what the user wishes. > > > > I don't know which of NTF-flags and NLM-flags/NUD-state should be given > > high priority on setting. For now, in bridge, NTF_USE masks any other > > flags. If this is not proper way for netlink/neighbour, I will agree > > with you. If not sure, I have no motivation to change existing behavior > > that might be expected by some users. > > > >> > >> It also provides duplicate functionality. The same results are achieved > >> by setting NLM_F_CREATE flag and NUD_REACHABLE state in the message. > > > > br_fdb_update() never updates fdb->used, which is visible by user, > > unlike fdb_add_entry(). > > Thanks for pointing this out. It looks like there are some > inconsistencies in the fdb->used markings as well. > > > > > If it is duplicate functionality, isn't NTF_USE itself no use? > > What can be achieved by changing capability of creation and update of > > local entries? > > Update of local entries gives you port redirection, but doing it under > rcu. Not sure if it really makes much sense though... > > NTF_USE makes since for neighbor cache as it triggers an ARP and a > refresh of the entry. Suppose, NTF_USE on the fdb entry should > trigger a refresh as well, but causing a create has to be explicit. > > I think I'll just send my patch and we can continue this discussion > there. OK, I'll wait for your patch. Thanks, Toshiaki Makita