On Wed, May 27, 2015 at 12:05 AM, Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> wrote: > On Tue, May 26, 2015 at 7:28 PM, Stephen Hemminger > <stephen@xxxxxxxxxxxxxxxxxx> wrote: >> On Thu, 21 May 2015 03:42:57 -0700 >> Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> wrote: >> >>> From: Wilson Kok <wkok@xxxxxxxxxxxxxxxxxxx> >>> >>> Check in fdb_add_entry() if the source port should learn, similar >>> check is used in br_fdb_update. >>> Note that new fdb entries which are added manually or >>> as local ones are still permitted. >>> This patch has been tested by running traffic via a bridge port and >>> switching the port's state, also by manually adding/removing entries >>> from the bridge's fdb. >>> >>> Signed-off-by: Wilson Kok <wkok@xxxxxxxxxxxxxxxxxxx> >>> Signed-off-by: Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> >> >> What is the problem this is trying to solve? >> >> I think user should be allowed to manually add any entry >> even if learning. > > Hi Stephen, > I have been thinking about the use case and have discussed it > internally with colleagues and the patch > author, the main problem is when there's an external software that > adds dynamic entries (learning) and > it could experience a race condition, here's a possible situation: > * external software learns a mac from hw, sends an add to kernel > * right before that, port goes blocking (or down) and kernel flushes > mac, sends notification about the state change and mac flush > * right after that, kernel gets the previous add from external software, it's > allowed to add, and then sends an add notification > * mean while, external software processes the link block/down and mac flush, > followed by the mac add from kernel. At this point, external software can't > really know whether it's a user adding the mac intentionally or it's > a race. > > This issue can't really be avoided in user-space. > As I've noted local and static entries are still allowed, and iproute2 > bridge utility always > marks the entries as static (NUD_NOARP), this only affects external > dynamic entries which > are usually sent by something that does the learning externally. > I'll keep digging to see if there's another way to go about this since > I'd like to give the user > full freedom. Personally I don't have strong feeling for this patch > and if it's not preferred then > I'll post a revert. So there is already a switchdev API to add/del an externally learned FDB entry which holds rtnl_lock and avoids these races. I would suggest using that and revert this patch. See call_switchdev_notifiers(SWITCHDEV_FDB_ADD|SWITCHDEV_FDB_DEL) and the handler in br.c:br_switchdev_event(). -scott
