[Bridge] [PATCH net-next] bridge: skip fdb add if the port shouldn't learn

[Scott Feldman]

On Wed, May 27, 2015 at 9:14 AM, Nikolay Aleksandrov
<nikolay at cumulusnetworks.com> wrote:
> On Wed, May 27, 2015 at 6:01 PM, Scott Feldman <sfeldma at gmail.com> wrote:
>> On Wed, May 27, 2015 at 1:35 AM, Nikolay Aleksandrov
>> <nikolay at cumulusnetworks.com> wrote:
>>> On Wed, May 27, 2015 at 9:59 AM, Scott Feldman <sfeldma at gmail.com> wrote:
>>>> On Wed, May 27, 2015 at 12:05 AM, Nikolay Aleksandrov
>>>> <nikolay at cumulusnetworks.com> wrote:
>>>>> On Tue, May 26, 2015 at 7:28 PM, Stephen Hemminger
>>>>> <stephen at networkplumber.org> wrote:
>>>>>> On Thu, 21 May 2015 03:42:57 -0700
>>>>>> Nikolay Aleksandrov <nikolay at cumulusnetworks.com> wrote:
>>>>>>
>>>>>>> From: Wilson Kok <wkok at cumulusnetworks.com>
>>>>>>>
>>>>>>> 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 at cumulusnetworks.com>
>>>>>>> Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
>>>>>>
>>>>>> 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