From: Nikolay Aleksandrov <razor@blackwall.org> To: Hans Schultz <schultz.hans@gmail.com>, davem@davemloft.net, kuba@kernel.org Cc: netdev@vger.kernel.org, Hans Schultz <schultz.hans+netdev@gmail.com>, Andrew Lunn <andrew@lunn.ch>, Vivien Didelot <vivien.didelot@gmail.com>, Florian Fainelli <f.fainelli@gmail.com>, Vladimir Oltean <olteanv@gmail.com>, Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Jiri Pirko <jiri@resnulli.us>, Ivan Vecera <ivecera@redhat.com>, Roopa Prabhu <roopa@nvidia.com>, Shuah Khan <shuah@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Ido Schimmel <idosch@nvidia.com>, linux-kernel@vger.kernel.org, bridge@lists.linux-foundation.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature Date: Tue, 24 May 2022 18:39:02 +0300 [thread overview] Message-ID: <01e6e35c-f5c9-9776-1263-058f84014ed9@blackwall.org> (raw) In-Reply-To: <20220524152144.40527-2-schultz.hans+netdev@gmail.com> 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 <schultz.hans+netdev@gmail.com> > --- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Nikolay Aleksandrov <razor@blackwall.org> To: Hans Schultz <schultz.hans@gmail.com>, davem@davemloft.net, kuba@kernel.org Cc: Ivan Vecera <ivecera@redhat.com>, Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, Jiri Pirko <jiri@resnulli.us>, Daniel Borkmann <daniel@iogearbox.net>, netdev@vger.kernel.org, Ido Schimmel <idosch@nvidia.com>, bridge@lists.linux-foundation.org, Eric Dumazet <edumazet@google.com>, linux-kernel@vger.kernel.org, Hans Schultz <schultz.hans+netdev@gmail.com>, linux-kselftest@vger.kernel.org, Roopa Prabhu <roopa@nvidia.com>, Paolo Abeni <pabeni@redhat.com>, Vladimir Oltean <olteanv@gmail.com>, Shuah Khan <shuah@kernel.org>, Vivien Didelot <vivien.didelot@gmail.com> Subject: Re: [Bridge] [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature Date: Tue, 24 May 2022 18:39:02 +0300 [thread overview] Message-ID: <01e6e35c-f5c9-9776-1263-058f84014ed9@blackwall.org> (raw) In-Reply-To: <20220524152144.40527-2-schultz.hans+netdev@gmail.com> 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 <schultz.hans+netdev@gmail.com> > --- > 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
next prev parent reply other threads:[~2022-05-24 15:39 UTC|newest] Thread overview: 108+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-24 15:21 [PATCH V3 net-next 0/4] Extend locked port feature with FDB locked flag (MAC-Auth/MAB) Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-24 15:21 ` [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-24 15:39 ` Nikolay Aleksandrov [this message] 2022-05-24 15:39 ` Nikolay Aleksandrov 2022-05-24 16:08 ` Hans Schultz 2022-05-24 16:08 ` [Bridge] " Hans Schultz 2022-05-24 16:21 ` Hans Schultz 2022-05-24 16:21 ` [Bridge] " Hans Schultz 2022-05-25 8:06 ` Nikolay Aleksandrov 2022-05-25 8:06 ` [Bridge] " Nikolay Aleksandrov 2022-05-25 8:34 ` Hans Schultz 2022-05-25 8:34 ` [Bridge] " Hans Schultz 2022-05-25 8:38 ` Nikolay Aleksandrov 2022-05-25 8:38 ` [Bridge] " Nikolay Aleksandrov 2022-05-25 9:11 ` Hans Schultz 2022-05-25 9:11 ` [Bridge] " Hans Schultz 2022-05-25 10:18 ` Nikolay Aleksandrov 2022-05-25 10:18 ` [Bridge] " Nikolay Aleksandrov 2022-07-06 18:13 ` Vladimir Oltean 2022-07-06 18:13 ` [Bridge] " Vladimir Oltean 2022-07-06 19:38 ` Nikolay Aleksandrov 2022-07-06 19:38 ` [Bridge] " Nikolay Aleksandrov 2022-07-06 20:21 ` Vladimir Oltean 2022-07-06 20:21 ` [Bridge] " Vladimir Oltean 2022-07-06 21:01 ` Nikolay Aleksandrov 2022-07-06 21:01 ` [Bridge] " Nikolay Aleksandrov 2022-07-07 14:08 ` Nikolay Aleksandrov 2022-07-07 14:08 ` [Bridge] " Nikolay Aleksandrov 2022-07-07 17:15 ` Vladimir Oltean 2022-07-07 17:15 ` [Bridge] " Vladimir Oltean 2022-07-07 17:26 ` Nikolay Aleksandrov 2022-07-07 17:26 ` [Bridge] " Nikolay Aleksandrov 2022-07-08 6:38 ` Hans S 2022-07-08 6:38 ` [Bridge] " Hans S 2022-05-26 14:13 ` Ido Schimmel 2022-05-26 14:13 ` [Bridge] " Ido Schimmel 2022-05-27 8:52 ` Hans Schultz 2022-05-27 8:52 ` [Bridge] " Hans Schultz 2022-05-27 9:58 ` Ido Schimmel 2022-05-27 9:58 ` [Bridge] " Ido Schimmel 2022-05-27 16:00 ` Hans Schultz 2022-05-27 16:00 ` [Bridge] " Hans Schultz 2022-05-31 9:34 ` Hans Schultz 2022-05-31 9:34 ` [Bridge] " Hans Schultz 2022-05-31 14:23 ` Ido Schimmel 2022-05-31 14:23 ` [Bridge] " Ido Schimmel 2022-05-31 15:49 ` Hans Schultz 2022-05-31 15:49 ` [Bridge] " Hans Schultz 2022-06-02 9:17 ` Hans Schultz 2022-06-02 9:17 ` [Bridge] " Hans Schultz 2022-06-02 9:33 ` Nikolay Aleksandrov 2022-06-02 9:33 ` [Bridge] " Nikolay Aleksandrov 2022-06-02 10:17 ` Hans Schultz 2022-06-02 10:17 ` [Bridge] " Hans Schultz 2022-06-02 10:30 ` Nikolay Aleksandrov 2022-06-02 10:30 ` [Bridge] " Nikolay Aleksandrov 2022-06-02 10:39 ` Ido Schimmel 2022-06-02 10:39 ` [Bridge] " Ido Schimmel 2022-06-02 11:36 ` Hans Schultz 2022-06-02 11:36 ` [Bridge] " Hans Schultz 2022-06-02 11:55 ` Ido Schimmel 2022-06-02 11:55 ` [Bridge] " Ido Schimmel 2022-06-02 12:08 ` Hans Schultz 2022-06-02 12:08 ` [Bridge] " Hans Schultz 2022-06-02 12:18 ` Ido Schimmel 2022-06-02 12:18 ` [Bridge] " Ido Schimmel 2022-06-02 12:53 ` Hans S 2022-06-02 13:27 ` Hans S 2022-06-02 13:27 ` [Bridge] " Hans S 2022-05-24 15:21 ` [PATCH V3 net-next 2/4] net: switchdev: add support for offloading of fdb locked flag Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-06-27 16:06 ` Vladimir Oltean 2022-06-27 16:06 ` [Bridge] " Vladimir Oltean 2022-05-24 15:21 ` [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-24 21:36 ` kernel test robot 2022-06-27 12:58 ` Hans S 2022-06-27 12:58 ` [Bridge] " Hans S 2022-06-27 18:05 ` Vladimir Oltean 2022-06-27 18:05 ` [Bridge] " Vladimir Oltean 2022-06-28 12:26 ` Hans S 2022-06-28 12:26 ` [Bridge] " Hans S 2022-07-05 15:05 ` Hans S 2022-07-05 15:05 ` [Bridge] " Hans S 2022-07-06 13:28 ` Vladimir Oltean 2022-07-06 13:28 ` [Bridge] " Vladimir Oltean 2022-07-06 13:48 ` Hans S 2022-07-06 13:48 ` [Bridge] " Hans S 2022-07-06 8:55 ` Vladimir Oltean 2022-07-06 8:55 ` [Bridge] " Vladimir Oltean 2022-07-06 10:12 ` Hans S 2022-07-06 10:12 ` [Bridge] " Hans S 2022-07-06 14:23 ` Hans S 2022-07-06 14:23 ` [Bridge] " Hans S 2022-07-06 14:33 ` Vladimir Oltean 2022-07-06 14:33 ` [Bridge] " Vladimir Oltean 2022-07-06 15:38 ` Hans S 2022-07-06 15:38 ` [Bridge] " Hans S 2022-07-07 6:54 ` Hans S 2022-07-07 6:54 ` [Bridge] " Hans S 2022-05-24 15:21 ` [PATCH V3 net-next 4/4] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-26 14:27 ` Ido Schimmel 2022-05-26 14:27 ` [Bridge] " Ido Schimmel 2022-05-27 9:07 ` Hans Schultz 2022-05-27 9:07 ` [Bridge] " Hans Schultz
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=01e6e35c-f5c9-9776-1263-058f84014ed9@blackwall.org \ --to=razor@blackwall.org \ --cc=andrew@lunn.ch \ --cc=bridge@lists.linux-foundation.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=f.fainelli@gmail.com \ --cc=idosch@nvidia.com \ --cc=ivecera@redhat.com \ --cc=jiri@resnulli.us \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=olteanv@gmail.com \ --cc=pabeni@redhat.com \ --cc=roopa@nvidia.com \ --cc=schultz.hans+netdev@gmail.com \ --cc=schultz.hans@gmail.com \ --cc=shuah@kernel.org \ --cc=vivien.didelot@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.