| Message ID | 20220317093902.1305816-2-schultz.hans+netdev@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Extend locked port feature with FDB locked flag (MAC-Auth/MAB) | expand |
On 17/03/2022 11:38, 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. > 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 <schultz.hans+netdev@gmail.com> > --- > include/uapi/linux/neighbour.h | 1 + > net/bridge/br_fdb.c | 6 ++++++ > net/bridge/br_input.c | 10 +++++++++- > net/bridge/br_private.h | 3 ++- > 4 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h > index db05fb55055e..a2df3b9b2f68 100644 > --- a/include/uapi/linux/neighbour.h > +++ b/include/uapi/linux/neighbour.h > @@ -51,6 +51,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 6ccda68bd473..57ec559a85a7 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; > + u8 ext_flags = 0; Let's not limit ourselves to 8 bits only for new flags, we'll need to extend them very soon. The neigh code defines the attribute as u32 which seems more appropriate. > > 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_u8(skb, NDA_FLAGS_EXT, ext_flags)) > + goto nla_put_failure; > + You have to account for the new attribute size in fdb_nlmsg_size(). > ci.ndm_used = jiffies_to_clock_t(now - fdb->used); > ci.ndm_confirmed = 0; > ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated); > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index e0c13fcc50ed..5ef25a496806 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -94,8 +94,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); > > if (!fdb_src || READ_ONCE(fdb_src->dst) != p || > - test_bit(BR_FDB_LOCAL, &fdb_src->flags)) > + test_bit(BR_FDB_LOCAL, &fdb_src->flags) || > + test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) { > + if (!fdb_src) { > + unsigned long flags = 0; > + > + set_bit(BR_FDB_ENTRY_LOCKED, &flags); __set_bit() > + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags); > + } > goto drop; > + } > } > > nbp_switchdev_frame_mark(p, skb); > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 48bc61ebc211..f5a0b68c4857 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -248,7 +248,8 @@ enum { > BR_FDB_ADDED_BY_EXT_LEARN, > BR_FDB_OFFLOADED, > BR_FDB_NOTIFY, > - BR_FDB_NOTIFY_INACTIVE > + BR_FDB_NOTIFY_INACTIVE, > + BR_FDB_ENTRY_LOCKED, > }; > > struct net_bridge_fdb_key {
On Thu, Mar 17, 2022 at 10:38:59AM +0100, 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. > Only the kernel can set this FDB entry flag, while userspace can read > the flag and remove it by deleting the FDB entry. Can you explain where this flag is rejected by the kernel? Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for new flags we should do a better job and reject unsupported configurations. WDYT? The neighbour code will correctly reject the new flag due to 'NTF_EXT_MASK'.
On 17/03/2022 15:44, Ido Schimmel wrote: > On Thu, Mar 17, 2022 at 10:38:59AM +0100, 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. >> Only the kernel can set this FDB entry flag, while userspace can read >> the flag and remove it by deleting the FDB entry. > > Can you explain where this flag is rejected by the kernel? > > Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for > new flags we should do a better job and reject unsupported > configurations. WDYT? > Definitely, I agree. > The neighbour code will correctly reject the new flag due to > 'NTF_EXT_MASK'.
On tor, mar 17, 2022 at 15:44, Ido Schimmel <idosch@idosch.org> wrote: > On Thu, Mar 17, 2022 at 10:38:59AM +0100, 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. >> Only the kernel can set this FDB entry flag, while userspace can read >> the flag and remove it by deleting the FDB entry. > > Can you explain where this flag is rejected by the kernel? > Is it an effort to set the flag from iproute2 on adding a fdb entry? > Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for > new flags we should do a better job and reject unsupported > configurations. WDYT? > > The neighbour code will correctly reject the new flag due to > 'NTF_EXT_MASK'.
On Thu, Mar 17, 2022 at 03:50:26PM +0100, Hans Schultz wrote: > On tor, mar 17, 2022 at 15:44, Ido Schimmel <idosch@idosch.org> wrote: > > On Thu, Mar 17, 2022 at 10:38:59AM +0100, 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. > >> Only the kernel can set this FDB entry flag, while userspace can read > >> the flag and remove it by deleting the FDB entry. > > > > Can you explain where this flag is rejected by the kernel? > > > Is it an effort to set the flag from iproute2 on adding a fdb entry? I'm not sure what you are asking, but even if iproute2 can't set the flag it doesn't mean the kernel shouldn't reject it > > > Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for > > new flags we should do a better job and reject unsupported > > configurations. WDYT? > > > > The neighbour code will correctly reject the new flag due to > > 'NTF_EXT_MASK'.
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h index db05fb55055e..a2df3b9b2f68 100644 --- a/include/uapi/linux/neighbour.h +++ b/include/uapi/linux/neighbour.h @@ -51,6 +51,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 6ccda68bd473..57ec559a85a7 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; + u8 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_u8(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); diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index e0c13fcc50ed..5ef25a496806 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -94,8 +94,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); if (!fdb_src || READ_ONCE(fdb_src->dst) != p || - test_bit(BR_FDB_LOCAL, &fdb_src->flags)) + test_bit(BR_FDB_LOCAL, &fdb_src->flags) || + test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) { + if (!fdb_src) { + unsigned long flags = 0; + + set_bit(BR_FDB_ENTRY_LOCKED, &flags); + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags); + } goto drop; + } } nbp_switchdev_frame_mark(p, skb); diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 48bc61ebc211..f5a0b68c4857 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -248,7 +248,8 @@ enum { BR_FDB_ADDED_BY_EXT_LEARN, BR_FDB_OFFLOADED, BR_FDB_NOTIFY, - BR_FDB_NOTIFY_INACTIVE + BR_FDB_NOTIFY_INACTIVE, + BR_FDB_ENTRY_LOCKED, }; struct net_bridge_fdb_key {
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. 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 <schultz.hans+netdev@gmail.com> --- include/uapi/linux/neighbour.h | 1 + net/bridge/br_fdb.c | 6 ++++++ net/bridge/br_input.c | 10 +++++++++- net/bridge/br_private.h | 3 ++- 4 files changed, 18 insertions(+), 2 deletions(-)