On Wed, May 27, 2015 at 9:14 AM, Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> wrote: > On Wed, May 27, 2015 at 6:01 PM, Scott Feldman <sfeldma@xxxxxxxxx> 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. >> > > I meant between externally learned entries from a user-space daemon and manually > added by the user. > >>> 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 got the API, but it doesn't help in the situation I explained > because it's a user-space > software that adds the externally learned entry, so I'm talking about > differentiating between externally learned dynamic entry from a device > which doesn't have > a kernel driver and can't call these notifiers, thus if we disallow > such dynamic entries when > the port is not in the proper state helps to both close the race and > fix the problem. IMO, we should not be adding weird patches like this to the kernel to support the out-of-kernel user-space switch drivers (SDK). This patch is trying to workaround a serialization issue with netlink messages created by you because you're using a closed-source user-space driver. It took us a couple of email replies to draw out your use-case, and if someone down the road tries to figure out what this patch is doing, it's not going to be obvious from the kernel code. I feel this patch should be reverted unless someone can show how it can be useful in another context. -scotta
