On 24/05/2022 18:21, Hans Schultz wrote:
> Add an intermediate state for clients behind a locked port to allow for
> possible opening of the port for said clients. This feature corresponds
> to the Mac-Auth and MAC Authentication Bypass (MAB) named features. The
> latter defined by Cisco.
> Locked FDB entries will be limited in number, so as to prevent DOS
> attacks by spamming the port with random entries. The limit will be
> a per port limit as it is a port based feature and that the port flushes
> all FDB entries on link down.
>
> Only the kernel can set this FDB entry flag, while userspace can read
> the flag and remove it by deleting the FDB entry.
>
> Signed-off-by: Hans Schultz <[email protected]>
> ---
> include/uapi/linux/neighbour.h | 1 +
> net/bridge/br_fdb.c | 11 +++++++++++
> net/bridge/br_if.c | 1 +
> net/bridge/br_input.c | 11 ++++++++++-
> net/bridge/br_private.h | 7 ++++++-
> 5 files changed, 29 insertions(+), 2 deletions(-)
>
Hi Hans,
So this approach has a fundamental problem, f->dst is changed without any synchronization
you cannot rely on it and thus you cannot account for these entries properly. We must be very
careful if we try to add any new synchronization not to affect performance as well.
More below...
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 39c565e460c7..76d65b481086 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -53,6 +53,7 @@ enum {
> #define NTF_ROUTER (1 << 7)
> /* Extended flags under NDA_FLAGS_EXT: */
> #define NTF_EXT_MANAGED (1 << 0)
> +#define NTF_EXT_LOCKED (1 << 1)
>
> /*
> * Neighbor Cache Entry States.
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index e7f4fccb6adb..6b83e2d6435d 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -105,6 +105,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
> struct nda_cacheinfo ci;
> struct nlmsghdr *nlh;
> struct ndmsg *ndm;
> + u32 ext_flags = 0;
>
> nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
> if (nlh == NULL)
> @@ -125,11 +126,16 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
> ndm->ndm_flags |= NTF_EXT_LEARNED;
> if (test_bit(BR_FDB_STICKY, &fdb->flags))
> ndm->ndm_flags |= NTF_STICKY;
> + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))
> + ext_flags |= NTF_EXT_LOCKED;
>
> if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
> goto nla_put_failure;
> if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
> goto nla_put_failure;
> + if (nla_put_u32(skb, NDA_FLAGS_EXT, ext_flags))
> + goto nla_put_failure;
> +
> ci.ndm_used = jiffies_to_clock_t(now - fdb->used);
> ci.ndm_confirmed = 0;
> ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated);
> @@ -171,6 +177,7 @@ static inline size_t fdb_nlmsg_size(void)
> return NLMSG_ALIGN(sizeof(struct ndmsg))
> + nla_total_size(ETH_ALEN) /* NDA_LLADDR */
> + nla_total_size(sizeof(u32)) /* NDA_MASTER */
> + + nla_total_size(sizeof(u32)) /* NDA_FLAGS_EXT */
> + nla_total_size(sizeof(u16)) /* NDA_VLAN */
> + nla_total_size(sizeof(struct nda_cacheinfo))
> + nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
> if (test_bit(BR_FDB_STATIC, &f->flags))
> fdb_del_hw_addr(br, f->key.addr.addr);
>
> + if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags) && !test_bit(BR_FDB_OFFLOADED, &f->flags))
> + atomic_dec(&f->dst->locked_entry_cnt);
Sorry but you cannot do this for multiple reasons:
- f->dst can be NULL
- f->dst changes without any synchronization
- there is no synchronization between fdb's flags and its ->dst
Cheers,
Nik
>
> Hi Hans,
> So this approach has a fundamental problem, f->dst is changed without any synchronization
> you cannot rely on it and thus you cannot account for these entries properly. We must be very
> careful if we try to add any new synchronization not to affect performance as well.
> More below...
>
>> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
>> if (test_bit(BR_FDB_STATIC, &f->flags))
>> fdb_del_hw_addr(br, f->key.addr.addr);
>>
>> + if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags) && !test_bit(BR_FDB_OFFLOADED, &f->flags))
>> + atomic_dec(&f->dst->locked_entry_cnt);
>
> Sorry but you cannot do this for multiple reasons:
> - f->dst can be NULL
> - f->dst changes without any synchronization
> - there is no synchronization between fdb's flags and its ->dst
>
> Cheers,
> Nik
Hi Nik,
I could check if f->dst is NULL, but in general this should be able to
work on a per port basis, so do you have an idea of how to keep a per
port counter of added locked fdb entries?
Best,
Hans
>
> Hi Hans,
> So this approach has a fundamental problem, f->dst is changed without any synchronization
> you cannot rely on it and thus you cannot account for these entries properly. We must be very
> careful if we try to add any new synchronization not to affect performance as well.
> More below...
>
>> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
>> if (test_bit(BR_FDB_STATIC, &f->flags))
>> fdb_del_hw_addr(br, f->key.addr.addr);
>>
>> + if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags) && !test_bit(BR_FDB_OFFLOADED, &f->flags))
>> + atomic_dec(&f->dst->locked_entry_cnt);
>
> Sorry but you cannot do this for multiple reasons:
> - f->dst can be NULL
> - f->dst changes without any synchronization
> - there is no synchronization between fdb's flags and its ->dst
>
> Cheers,
> Nik
Hi Nik,
if a port is decoupled from the bridge, the locked entries would of
course be invalid, so maybe if adding and removing a port is accounted
for wrt locked entries and the count of locked entries, would that not
work?
Best,
Hans
On 24/05/2022 19:21, Hans Schultz wrote:
>>
>> Hi Hans,
>> So this approach has a fundamental problem, f->dst is changed without any synchronization
>> you cannot rely on it and thus you cannot account for these entries properly. We must be very
>> careful if we try to add any new synchronization not to affect performance as well.
>> More below...
>>
>>> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
>>> if (test_bit(BR_FDB_STATIC, &f->flags))
>>> fdb_del_hw_addr(br, f->key.addr.addr);
>>>
>>> + if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags) && !test_bit(BR_FDB_OFFLOADED, &f->flags))
>>> + atomic_dec(&f->dst->locked_entry_cnt);
>>
>> Sorry but you cannot do this for multiple reasons:
>> - f->dst can be NULL
>> - f->dst changes without any synchronization
>> - there is no synchronization between fdb's flags and its ->dst
>>
>> Cheers,
>> Nik
>
> Hi Nik,
>
> if a port is decoupled from the bridge, the locked entries would of
> course be invalid, so maybe if adding and removing a port is accounted
> for wrt locked entries and the count of locked entries, would that not
> work?
>
> Best,
> Hans
Hi Hans,
Unfortunately you need the correct amount of locked entries per-port if you want
to limit their number per-port, instead of globally. So you need a consistent
fdb view with all its attributes when changing its dst in this case, which would
require new locking because you have multiple dependent struct fields and it will
kill roaming/learning scalability. I don't think this use case is worth the complexity it
will bring, so I'd suggest an alternative - you can monitor the number of locked entries
per-port from a user-space agent and disable port learning or some similar solution that
doesn't require any complex kernel changes. Is the limit a requirement to add the feature?
I have an idea how to do it and to minimize the performance hit if it really is needed
but it'll add a lot of complexity which I'd like to avoid if possible.
Cheers,
Nik
On ons, maj 25, 2022 at 11:06, Nikolay Aleksandrov <[email protected]> wrote:
> On 24/05/2022 19:21, Hans Schultz wrote:
>>>
>>> Hi Hans,
>>> So this approach has a fundamental problem, f->dst is changed without any synchronization
>>> you cannot rely on it and thus you cannot account for these entries properly. We must be very
>>> careful if we try to add any new synchronization not to affect performance as well.
>>> More below...
>>>
>>>> @@ -319,6 +326,9 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
>>>> if (test_bit(BR_FDB_STATIC, &f->flags))
>>>> fdb_del_hw_addr(br, f->key.addr.addr);
>>>>
>>>> + if (test_bit(BR_FDB_ENTRY_LOCKED, &f->flags) && !test_bit(BR_FDB_OFFLOADED, &f->flags))
>>>> + atomic_dec(&f->dst->locked_entry_cnt);
>>>
>>> Sorry but you cannot do this for multiple reasons:
>>> - f->dst can be NULL
>>> - f->dst changes without any synchronization
>>> - there is no synchronization between fdb's flags and its ->dst
>>>
>>> Cheers,
>>> Nik
>>
>> Hi Nik,
>>
>> if a port is decoupled from the bridge, the locked entries would of
>> course be invalid, so maybe if adding and removing a port is accounted
>> for wrt locked entries and the count of locked entries, would that not
>> work?
>>
>> Best,
>> Hans
>
> Hi Hans,
> Unfortunately you need the correct amount of locked entries per-port if you want
> to limit their number per-port, instead of globally. So you need a
> consistent
Hi Nik,
the used dst is a port structure, so it is per-port and not globally.
Best,
Hans
> fdb view with all its attributes when changing its dst in this case, which would
> require new locking because you have multiple dependent struct fields and it will
> kill roaming/learning scalability. I don't think this use case is worth the complexity it
> will bring, so I'd suggest an alternative - you can monitor the number of locked entries
> per-port from a user-space agent and disable port learning or some similar solution that
> doesn't require any complex kernel changes. Is the limit a requirement to add the feature?
>
> I have an idea how to do it and to minimize the performance hit if it really is needed
> but it'll add a lot of complexity which I'd like to avoid if possible.
>
> Cheers,
> Nik