Ido Schimmel
2022-Aug-27 15:19 UTC
[Bridge] [PATCH v5 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature
On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h > index a998bf761635..bc1440a56b70 100644 > --- a/include/uapi/linux/neighbour.h > +++ b/include/uapi/linux/neighbour.h > @@ -52,7 +52,9 @@ enum { > #define NTF_STICKY (1 << 6) > #define NTF_ROUTER (1 << 7) > /* Extended flags under NDA_FLAGS_EXT: */ > -#define NTF_EXT_MANAGED (1 << 0) > +#define NTF_EXT_MANAGED (1 << 0) > +#define NTF_EXT_LOCKED (1 << 1) > +#define NTF_EXT_BLACKHOLE (1 << 2)A few lines below in the file there is a comment explaining NTF_EXT_MANAGED. Please document NTF_EXT_LOCKED and NTF_EXT_BLACKHOLE as well.> > /* > * Neighbor Cache Entry States.[...]> @@ -1082,6 +1095,16 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, > modified = true; > } > > + if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) { > + clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags); > + modified = true; > + }Should be able to use test_and_clear_bit(): diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index e7f4fccb6adb..e5561ee2bfac 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -1082,6 +1082,9 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, modified = true; } + if (test_and_clear_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags)) + modified = true; + if (fdb_handle_notify(fdb, notify)) modified = true;> + > + if (test_bit(BR_FDB_BLACKHOLE, &fdb->flags)) { > + clear_bit(BR_FDB_BLACKHOLE, &fdb->flags); > + modified = true; > + }This will need to change to allow user space to set the flag.> + > if (fdb_handle_notify(fdb, notify)) > modified = true; > > @@ -1178,6 +1201,12 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[], > vg = nbp_vlan_group(p); > } > > + if (tb[NDA_FLAGS_EXT] && > + (nla_get_u32(tb[NDA_FLAGS_EXT]) & (NTF_EXT_LOCKED | NTF_EXT_BLACKHOLE))) { > + pr_info("bridge: RTM_NEWNEIGH has invalid extended flags\n"); > + return -EINVAL; > + } > + > if (tb[NDA_FDB_EXT_ATTRS]) { > attr = tb[NDA_FDB_EXT_ATTRS]; > err = nla_parse_nested(nfea_tb, NFEA_MAX, attr, > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 68b3e850bcb9..3d48aa7fa778 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -110,8 +110,19 @@ 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 || (READ_ONCE(fdb_src->dst) != p && > + (p->flags & BR_LEARNING))) {It looks like you are allowing a locked port to: 1. Overtake a local entry. Actually, it will be rejected by br_fdb_update() with a rate limited error message, but best to avoid it. 2. Overtake an entry pointing to an unlocked port. There is no reason for an authorized port to lose communication because an unauthorized port decided to spoof its MAC.> + unsigned long flags = 0; > + > + if (p->flags & BR_PORT_MAB) { > + __set_bit(BR_FDB_ENTRY_LOCKED, &flags); > + br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags); > + } > + } > goto drop; > + } > }How about the below (untested): diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 68b3e850bcb9..9143a94a1c57 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -109,9 +109,18 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb struct net_bridge_fdb_entry *fdb_src 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)) + if (!fdb_src) { + if (p->flags & BR_PORT_MAB) { + __set_bit(BR_FDB_ENTRY_LOCKED, &flags); + br_fdb_update(br, p, eth_hdr(skb)->h_source, + vid, flags); + } + goto drop; + } else if (READ_ONCE(fdb_src->dst) != p || + test_bit(BR_FDB_LOCAL, &fdb_src->flags) || + test_bit(BR_FDB_LOCKED, &fdb_src->flags)) { goto drop; + } } The semantics are very clear, IMO. On FDB miss, add a locked FDB entry and drop the packet. On FDB mismatch, drop the packet. Entry can roam from an unauthorized port to an authorized port, but not the other way around. Not sure what is the use case for allowing roaming between unauthorized ports. Note that with the above, locked entries are not refreshed and will therefore age out unless replaced by user space.> > nbp_switchdev_frame_mark(p, skb); > @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], > br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); > br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); > br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); > + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB); > + > + if (!(p->flags & BR_PORT_LOCKED)) > + p->flags &= ~BR_PORT_MAB;Any reason not to emit an error if MAB is enabled while the port is unlocked? Something like this (untested): diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 5aeb3646e74c..18353a4c29e1 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -944,6 +944,12 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); + if (!(p->flags & BR_PORT_LOCKED) && (p->flags & BR_PORT_MAB)) { + NL_SET_ERR_MSG(extack, "MAB cannot be enabled when port is unlocked"); + p->flags = old_flags; + return -EINVAL; + } + changed_mask = old_flags ^ p->flags; err = br_switchdev_set_port_flag(p, p->flags, changed_mask, extack);> > changed_mask = old_flags ^ p->flags; > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 06e5f6faa431..048e4afbc5a0 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -251,7 +251,9 @@ 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, > + BR_FDB_BLACKHOLE, > }; > > struct net_bridge_fdb_key { > -- > 2.30.2 >
netdev at kapio-technology.com
2022-Aug-28 10:23 UTC
[Bridge] [PATCH v5 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature
On 2022-08-27 17:19, Ido Schimmel wrote:> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote: >> >> nbp_switchdev_frame_mark(p, skb); >> @@ -943,6 +946,10 @@ static int br_setport(struct net_bridge_port *p, >> struct nlattr *tb[], >> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, >> BR_NEIGH_SUPPRESS); >> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); >> br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); >> + br_set_port_flag(p, tb, IFLA_BRPORT_MAB, BR_PORT_MAB); >> + >> + if (!(p->flags & BR_PORT_LOCKED)) >> + p->flags &= ~BR_PORT_MAB;The reason for this is that I wanted it to be so that if you have MAB enabled (and locked of course) and unlock the port, it will automatically clear both flags instead of having to first disable MAB and then unlock the port.> > Any reason not to emit an error if MAB is enabled while the port is > unlocked? Something like this (untested): > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 5aeb3646e74c..18353a4c29e1 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -944,6 +944,12 @@ static int br_setport(struct net_bridge_port *p, > struct nlattr *tb[], > br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); > br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); > > + if (!(p->flags & BR_PORT_LOCKED) && (p->flags & BR_PORT_MAB)) { > + NL_SET_ERR_MSG(extack, "MAB cannot be enabled when > port is unlocked"); > + p->flags = old_flags; > + return -EINVAL; > + } > + > changed_mask = old_flags ^ p->flags; > > err = br_switchdev_set_port_flag(p, p->flags, changed_mask, > extack); >
netdev at kapio-technology.com
2022-Aug-29 09:32 UTC
[Bridge] [PATCH v5 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature
On 2022-08-27 17:19, Ido Schimmel wrote:> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote: > How about the below (untested): > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 68b3e850bcb9..9143a94a1c57 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -109,9 +109,18 @@ int br_handle_frame_finish(struct net *net, > struct sock *sk, struct sk_buff *skb > struct net_bridge_fdb_entry *fdb_src > 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)) > + if (!fdb_src) { > + if (p->flags & BR_PORT_MAB) { > + __set_bit(BR_FDB_ENTRY_LOCKED, &flags); > + br_fdb_update(br, p, > eth_hdr(skb)->h_source, > + vid, flags); > + } > + goto drop; > + } else if (READ_ONCE(fdb_src->dst) != p || > + test_bit(BR_FDB_LOCAL, &fdb_src->flags) || > + test_bit(BR_FDB_LOCKED, &fdb_src->flags)) { > goto drop; > + } > } > > The semantics are very clear, IMO. On FDB miss, add a locked FDB entry > and drop the packet. On FDB mismatch, drop the packet. > > Entry can roam from an unauthorized port to an authorized port, but not > the other way around. Not sure what is the use case for allowing > roaming > between unauthorized ports. > > Note that with the above, locked entries are not refreshed and will > therefore age out unless replaced by user space. >Okay I was under the impression that entries should be able to roam freely between authorized and unauthorized ports in the bridge as long as the locked flag is on when roaming to the MAB enabled port. As you know roaming is not a big issue with mv88e6xxx. As I see this code, an entry cannot roam to an authorized port as there is no update after the port mismatch check and the packet is dropped as it should in this case in the locked section.
netdev at kapio-technology.com
2022-Aug-29 11:01 UTC
[Bridge] [PATCH v5 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature
On 2022-08-27 17:19, Ido Schimmel wrote:> On Fri, Aug 26, 2022 at 01:45:33PM +0200, Hans Schultz wrote:How about this? diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 1064a5b2d478..82bb50851716 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -103,8 +103,19 @@ 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 || ((READ_ONCE(fdb_src->dst) != p) && + (!unlikely(test_bit(BR_FDB_LOCAL, &fdb_src->flags))))) { + unsigned long flags = 0; + + if (p->flags & BR_PORT_MAB) { + __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); It will allow roaming to a MAB enabled port (no roaming to a simply locked port should be allowed of course), and it will not change a local entry and not rely on 'learning on' on the locked port of course. Roaming to an unlocked port will also be allowed, and the locked flag will be removed in this case according to code in br_fdb_update().
netdev at kapio-technology.com
2022-Aug-29 11:34 UTC
[Bridge] [PATCH v5 net-next 1/6] net: bridge: add locked entry fdb flag to extend locked port feature
On 2022-08-27 17:19, Ido Schimmel wrote:> > How about the below (untested): > > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index 68b3e850bcb9..9143a94a1c57 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -109,9 +109,18 @@ int br_handle_frame_finish(struct net *net, > struct sock *sk, struct sk_buff *skb > struct net_bridge_fdb_entry *fdb_src > 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)) > + if (!fdb_src) { > + if (p->flags & BR_PORT_MAB) { > + __set_bit(BR_FDB_ENTRY_LOCKED, &flags); > + br_fdb_update(br, p, > eth_hdr(skb)->h_source, > + vid, flags); > + } > + goto drop; > + } else if (READ_ONCE(fdb_src->dst) != p || > + test_bit(BR_FDB_LOCAL, &fdb_src->flags) || > + test_bit(BR_FDB_LOCKED, &fdb_src->flags)) { > goto drop; > + } > } > > The semantics are very clear, IMO. On FDB miss, add a locked FDB entry > and drop the packet. On FDB mismatch, drop the packet. > > Entry can roam from an unauthorized port to an authorized port, but not > the other way around. Not sure what is the use case for allowing > roaming > between unauthorized ports. > > Note that with the above, locked entries are not refreshed and will > therefore age out unless replaced by user space. >Okay, I got the semantics (locked/unlocked vs unauthorized/authorized) reversed, so I will go with your suggestion.