On Fri, Nov 08, 2024 at 04:24:19PM +1300, Elliot Ayrey wrote: > When an fdb entry is configured as static and sticky it should never > roam. However there are times where it would be useful to know when > this happens so a user application can act on it. For this reason, > extend the fdb notification mechanism to send a notification when the > bridge detects a host that is attempting to roam when it has been > configured not to. > > This is achieved by temporarily updating the fdb entry with the new > port, setting a new notify roaming bit, firing off a notification, and > restoring the original port immediately afterwards. The port remains > unchanged, respecting the sticky flag, but userspace is now notified > of the new port the host was seen on. > > The roaming bit is cleared if the entry becomes inactive or if it is > replaced by a user entry. > > Signed-off-by: Elliot Ayrey <[email protected]> > --- > include/uapi/linux/neighbour.h | 4 ++- > net/bridge/br_fdb.c | 64 +++++++++++++++++++++++----------- > net/bridge/br_input.c | 10 ++++-- > net/bridge/br_private.h | 3 ++ > 4 files changed, 58 insertions(+), 23 deletions(-) >
No way, this is ridiculous. Changing the port like that for a notification is not ok at all. It is also not the bridge's job to notify user-space for sticky fdbs that are trying to roam, you already have some user-space app and you can catch such fdbs by other means (sniffing, ebpf hooks, netfilter matching etc). Such change can also lead to DDoS attacks with many notifications. Nacked-by: Nikolay Aleksandrov <[email protected]> > diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h > index 5e67a7eaf4a7..e1c686268808 100644 > --- a/include/uapi/linux/neighbour.h > +++ b/include/uapi/linux/neighbour.h > @@ -201,10 +201,12 @@ enum { > /* FDB activity notification bits used in NFEA_ACTIVITY_NOTIFY: > * - FDB_NOTIFY_BIT - notify on activity/expire for any entry > * - FDB_NOTIFY_INACTIVE_BIT - mark as inactive to avoid multiple > notifications > + * - FDB_NOTIFY_ROAMING_BIT - mark as attempting to roam > */ > enum { > FDB_NOTIFY_BIT = (1 << 0), > - FDB_NOTIFY_INACTIVE_BIT = (1 << 1) > + FDB_NOTIFY_INACTIVE_BIT = (1 << 1), > + FDB_NOTIFY_ROAMING_BIT = (1 << 2) > }; > > /* embedded into NDA_FDB_EXT_ATTRS: > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index d0eeedc03390..a8b841e74e15 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -145,6 +145,8 @@ static int fdb_fill_info(struct sk_buff *skb, const > struct net_bridge *br, > goto nla_put_failure; > if (test_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags)) > notify_bits |= FDB_NOTIFY_INACTIVE_BIT; > + if (test_bit(BR_FDB_NOTIFY_ROAMING, &fdb->flags)) > + notify_bits |= FDB_NOTIFY_ROAMING_BIT; > > if (nla_put_u8(skb, NFEA_ACTIVITY_NOTIFY, notify_bits)) { > nla_nest_cancel(skb, nest); > @@ -554,8 +556,10 @@ void br_fdb_cleanup(struct work_struct *work) > work_delay = min(work_delay, > this_timer - now); > else if > (!test_and_set_bit(BR_FDB_NOTIFY_INACTIVE, > - &f->flags)) > + &f->flags)) { > + clear_bit(BR_FDB_NOTIFY_ROAMING, > &f->flags); > fdb_notify(br, f, RTM_NEWNEIGH, false); > + } > } > continue; > } > @@ -880,6 +884,19 @@ static bool __fdb_mark_active(struct > net_bridge_fdb_entry *fdb) > test_and_clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags)); > } > > +void br_fdb_notify_roaming(struct net_bridge *br, struct net_bridge_port *p, > + struct net_bridge_fdb_entry *fdb) > +{ > + struct net_bridge_port *old_p = READ_ONCE(fdb->dst); > + > + if (test_bit(BR_FDB_NOTIFY, &fdb->flags) && > + !test_and_set_bit(BR_FDB_NOTIFY_ROAMING, &fdb->flags)) { > + WRITE_ONCE(fdb->dst, p); > + fdb_notify(br, fdb, RTM_NEWNEIGH, false); > + WRITE_ONCE(fdb->dst, old_p); > + } > +} > + > void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, > const unsigned char *addr, u16 vid, unsigned long flags) > { > @@ -906,21 +923,24 @@ void br_fdb_update(struct net_bridge *br, struct > net_bridge_port *source, > } > > /* fastpath: update of existing entry */ > - if (unlikely(source != READ_ONCE(fdb->dst) && > - !test_bit(BR_FDB_STICKY, &fdb->flags))) { > - br_switchdev_fdb_notify(br, fdb, RTM_DELNEIGH); > - WRITE_ONCE(fdb->dst, source); > - fdb_modified = true; > - /* Take over HW learned entry */ > - if (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN, > - &fdb->flags))) > - clear_bit(BR_FDB_ADDED_BY_EXT_LEARN, > - &fdb->flags); > - /* Clear locked flag when roaming to an > - * unlocked port. > - */ > - if (unlikely(test_bit(BR_FDB_LOCKED, > &fdb->flags))) > - clear_bit(BR_FDB_LOCKED, &fdb->flags); > + if (unlikely(source != READ_ONCE(fdb->dst))) { > + if (unlikely(test_bit(BR_FDB_STICKY, > &fdb->flags))) { > + br_fdb_notify_roaming(br, source, fdb); > + } else { > + br_switchdev_fdb_notify(br, fdb, > RTM_DELNEIGH); > + WRITE_ONCE(fdb->dst, source); > + fdb_modified = true; > + /* Take over HW learned entry */ > + if > (unlikely(test_bit(BR_FDB_ADDED_BY_EXT_LEARN, > + &fdb->flags))) > + > clear_bit(BR_FDB_ADDED_BY_EXT_LEARN, > + &fdb->flags); > + /* Clear locked flag when roaming to an > + * unlocked port. > + */ > + if (unlikely(test_bit(BR_FDB_LOCKED, > &fdb->flags))) > + clear_bit(BR_FDB_LOCKED, > &fdb->flags); > + } > } > > if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags))) { > @@ -1045,6 +1065,7 @@ static bool fdb_handle_notify(struct > net_bridge_fdb_entry *fdb, u8 notify) > test_and_clear_bit(BR_FDB_NOTIFY, &fdb->flags)) { > /* disabled activity tracking, clear notify state */ > clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags); > + clear_bit(BR_FDB_NOTIFY_ROAMING, &fdb->flags); > modified = true; > } > > @@ -1457,10 +1478,13 @@ int br_fdb_external_learn_add(struct net_bridge *br, > struct net_bridge_port *p, > > fdb->updated = jiffies; > > - if (READ_ONCE(fdb->dst) != p && > - !test_bit(BR_FDB_STICK, &fdb->flags)) { > - WRITE_ONCE(fdb->dst, p); > - modified = true; > + if (READ_ONCE(fdb->dst) != p) { > + if (test_bit(BR_FDB_STICKY, &fdb->flags)) { > + br_fdb_notify_roaming(br, p, fdb); > + } else { > + WRITE_ONCE(fdb->dst, p); > + modified = true; > + } > } > > if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) { > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index ceaa5a89b947..512ffab16f5d 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -120,8 +120,14 @@ int br_handle_frame_finish(struct net *net, struct sock > *sk, struct sk_buff *skb > br_fdb_update(br, p, eth_hdr(skb)->h_source, > vid, BIT(BR_FDB_LOCKED)); > goto drop; > - } else if (READ_ONCE(fdb_src->dst) != p || > - test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { > + } else if (READ_ONCE(fdb_src->dst) != p) { > + /* FDB is trying to roam. Notify userspace and drop > + * the packet > + */ > + if (test_bit(BR_FDB_STICKY, &fdb_src->flags)) > + br_fdb_notify_roaming(br, p, fdb_src); > + goto drop; > + } else if (test_bit(BR_FDB_LOCAL, &fdb_src->flags)) { > /* FDB mismatch. Drop the packet without roaming. */ > goto drop; > } else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) { > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 041f6e571a20..18d3cb5fec0e 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -277,6 +277,7 @@ enum { > BR_FDB_NOTIFY_INACTIVE, > BR_FDB_LOCKED, > BR_FDB_DYNAMIC_LEARNED, > + BR_FDB_NOTIFY_ROAMING, > }; > > struct net_bridge_fdb_key { > @@ -874,6 +875,8 @@ int br_fdb_external_learn_del(struct net_bridge *br, > struct net_bridge_port *p, > bool swdev_notify); > void br_fdb_offloaded_set(struct net_bridge *br, struct net_bridge_port *p, > const unsigned char *addr, u16 vid, bool offloaded); > +void br_fdb_notify_roaming(struct net_bridge *br, struct net_bridge_port *p, > + struct net_bridge_fdb_entry *fdb); > > /* br_forward.c */ > enum br_pkt_type {
