Date:	Wed, 08 Jan 2014 15:02:36 +0900
From:	Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
To:	vyasevic@...hat.com
Cc:	Toshiaki Makita <toshiaki.makita1@...il.com>,
	"David S . Miller" <davem@...emloft.net>,
	Stephen Hemminger <stephen@...workplumber.org>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net v2 1/9] bridge: Fix the way to find old local fdb
 entries in br_fdb_changeaddr

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists