On Tue, Jun 2, 2015 at 10:14 AM, roopa <roopa@xxxxxxxxxxxxxxxxxxx> wrote: > On 5/27/15, 9:01 AM, Scott Feldman wrote: >> >> On Wed, May 27, 2015 at 1:35 AM, Nikolay Aleksandrov >> <nikolay@xxxxxxxxxxxxxxxxxxx> wrote: >>> >>> On Wed, May 27, 2015 at 9:59 AM, Scott Feldman <sfeldma@xxxxxxxxx> wrote: >>>> >>>> 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 >>> >>> Hmm, I'm new to the switchdev API and am possibly missing something, >>> but how do you suggest to use it here ? >> >> You need to call >> call_switchdev_notifiers(SWITCHDEV_FDB_ADD|SWITCHDEV_FDB_DEL) when the >> device learns a new mac/vlan on the port interface. >> >>> How can we differentiate between user-added entry and an externally >>> learned one ? >> >> Externally added ones will be marked with NTF_EXT_LEARNED set in >> ndm->ndm_flags in the netlink echo. Manually added ones from the user >> will have ndm->ndm_state set to NUD_NOARP in the netlink echo. >> >>> Do you mean to use (for example) the NTF_EXT_LEARNED flag when adding >>> an entry from user-space so >>> the API can get called in br_fdb_add ? >> >> No. br_fdb_add is the bridge's .ndo_fdb_add handler called when user >> manually adds an FDB entry using netlink RTM_NEWNEIGH. For externally >> learned entries, use the internal >> call_switchdev_notifiers(SWITCHDEV_FDB_ADD|SWITCHDEV_FDB_DEL). > > > scott, I am assuming you are ok with an external learning entity (user space > driver or a controller) pushing entries > with the NTF_EXT_LEARNED correct ?. Because NTF_EXT_LEARNED is in uapi (and > analogous to RTNH_F_OFFLOAD in the fib offload world IMO) That seems OK. I can see how that would remove the need for this patch, but still give you the control from user space daemon/listener to figure out what happened.
