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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 27 May 2015 18:14:47 +0200
From:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:	Scott Feldman <sfeldma@...il.com>
Cc:	Stephen Hemminger <stephen@...workplumber.org>,
	Netdev <netdev@...r.kernel.org>,
	Wilson Kok <wkok@...ulusnetworks.com>,
	David Miller <davem@...emloft.net>,
	bridge@...ts.linux-foundation.org,
	Jiří Pírko <jiri@...nulli.us>
Subject: Re: [PATCH net-next] bridge: skip fdb add if the port shouldn't learn

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