[PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Hans Schultz]

In a 802.1X scenario, clients connected to a bridge port shall not
be allowed to have traffic forwarded until fully authenticated.
A static fdb entry of the clients MAC address for the bridge port
unlocks the client and allows bidirectional communication.

This scenario is facilitated with setting the bridge port in locked
mode, which is also supported by various switchcore chipsets.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 include/linux/if_bridge.h    |  1 +
 include/uapi/linux/if_link.h |  1 +
 net/bridge/br_input.c        | 10 +++++++++-
 net/bridge/br_netlink.c      |  6 +++++-
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 509e18c7e740..3aae023a9353 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -58,6 +58,7 @@ struct br_ip_list {
 #define BR_MRP_LOST_CONT	BIT(18)
 #define BR_MRP_LOST_IN_CONT	BIT(19)
 #define BR_TX_FWD_OFFLOAD	BIT(20)
+#define BR_PORT_LOCKED		BIT(21)
 
 #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 6218f93f5c1a..8fa2648fbc83 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -532,6 +532,7 @@ enum {
 	IFLA_BRPORT_GROUP_FWD_MASK,
 	IFLA_BRPORT_NEIGH_SUPPRESS,
 	IFLA_BRPORT_ISOLATED,
+	IFLA_BRPORT_LOCKED,
 	IFLA_BRPORT_BACKUP_PORT,
 	IFLA_BRPORT_MRP_RING_OPEN,
 	IFLA_BRPORT_MRP_IN_OPEN,
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index b50382f957c1..469e3adbce07 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -69,6 +69,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
 	enum br_pkt_type pkt_type = BR_PKT_UNICAST;
 	struct net_bridge_fdb_entry *dst = NULL;
+	struct net_bridge_fdb_entry *fdb_entry;
 	struct net_bridge_mcast_port *pmctx;
 	struct net_bridge_mdb_entry *mdst;
 	bool local_rcv, mcast_hit = false;
@@ -81,6 +82,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (!p || p->state == BR_STATE_DISABLED)
 		goto drop;
 
+	br = p->br;
+
 	brmctx = &p->br->multicast_ctx;
 	pmctx = &p->multicast_ctx;
 	state = p->state;
@@ -88,10 +91,15 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 				&state, &vlan))
 		goto out;
 
+	if (p->flags & BR_PORT_LOCKED) {
+		fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
+		if (!(fdb_entry && fdb_entry->dst == p))
+			goto drop;
+	}
+
 	nbp_switchdev_frame_mark(p, skb);
 
 	/* insert into forwarding database after filtering to avoid spoofing */
-	br = p->br;
 	if (p->flags & BR_LEARNING)
 		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
 
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 2ff83d84230d..7d4432ca9a20 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -184,6 +184,7 @@ static inline size_t br_port_info_size(void)
 		+ nla_total_size(1)	/* IFLA_BRPORT_VLAN_TUNNEL */
 		+ nla_total_size(1)	/* IFLA_BRPORT_NEIGH_SUPPRESS */
 		+ nla_total_size(1)	/* IFLA_BRPORT_ISOLATED */
+		+ nla_total_size(1)	/* IFLA_BRPORT_LOCKED */
 		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_ROOT_ID */
 		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_BRIDGE_ID */
 		+ nla_total_size(sizeof(u16))	/* IFLA_BRPORT_DESIGNATED_PORT */
@@ -269,7 +270,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
 							  BR_MRP_LOST_CONT)) ||
 	    nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
 		       !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
-	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)))
+	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) ||
+	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)))
 		return -EMSGSIZE;
 
 	timerval = br_timer_value(&p->message_age_timer);
@@ -827,6 +829,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
 	[IFLA_BRPORT_GROUP_FWD_MASK] = { .type = NLA_U16 },
 	[IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
 	[IFLA_BRPORT_ISOLATED]	= { .type = NLA_U8 },
+	[IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
 	[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
 	[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
 };
@@ -893,6 +896,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
 	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
 	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);
 
 	changed_mask = old_flags ^ p->flags;
 
-- 
2.30.2

Re: [PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Ido Schimmel]

On Mon, Feb 07, 2022 at 11:07:39AM +0100, Hans Schultz wrote:
> In a 802.1X scenario, clients connected to a bridge port shall not
> be allowed to have traffic forwarded until fully authenticated.
> A static fdb entry of the clients MAC address for the bridge port
> unlocks the client and allows bidirectional communication.
> 
> This scenario is facilitated with setting the bridge port in locked
> mode, which is also supported by various switchcore chipsets.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  include/linux/if_bridge.h    |  1 +
>  include/uapi/linux/if_link.h |  1 +
>  net/bridge/br_input.c        | 10 +++++++++-
>  net/bridge/br_netlink.c      |  6 +++++-
>  4 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 509e18c7e740..3aae023a9353 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -58,6 +58,7 @@ struct br_ip_list {
>  #define BR_MRP_LOST_CONT	BIT(18)
>  #define BR_MRP_LOST_IN_CONT	BIT(19)
>  #define BR_TX_FWD_OFFLOAD	BIT(20)
> +#define BR_PORT_LOCKED		BIT(21)
>  
>  #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
>  
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 6218f93f5c1a..8fa2648fbc83 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -532,6 +532,7 @@ enum {
>  	IFLA_BRPORT_GROUP_FWD_MASK,
>  	IFLA_BRPORT_NEIGH_SUPPRESS,
>  	IFLA_BRPORT_ISOLATED,
> +	IFLA_BRPORT_LOCKED,

Please add it at the end to avoid breaking uAPI

>  	IFLA_BRPORT_BACKUP_PORT,
>  	IFLA_BRPORT_MRP_RING_OPEN,
>  	IFLA_BRPORT_MRP_IN_OPEN,
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index b50382f957c1..469e3adbce07 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -69,6 +69,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>  	enum br_pkt_type pkt_type = BR_PKT_UNICAST;
>  	struct net_bridge_fdb_entry *dst = NULL;
> +	struct net_bridge_fdb_entry *fdb_entry;
>  	struct net_bridge_mcast_port *pmctx;
>  	struct net_bridge_mdb_entry *mdst;
>  	bool local_rcv, mcast_hit = false;
> @@ -81,6 +82,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  	if (!p || p->state == BR_STATE_DISABLED)
>  		goto drop;
>  
> +	br = p->br;
> +
>  	brmctx = &p->br->multicast_ctx;
>  	pmctx = &p->multicast_ctx;
>  	state = p->state;
> @@ -88,10 +91,15 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>  				&state, &vlan))
>  		goto out;
>  
> +	if (p->flags & BR_PORT_LOCKED) {
> +		fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
> +		if (!(fdb_entry && fdb_entry->dst == p))
> +			goto drop;

I'm not familiar with 802.1X so I have some questions:

1. Do we need to differentiate between no FDB entry and an FDB entry
pointing to a different port than we expect?

2. Does user space care about SAs that did not pass the check? That is,
does it need to see notifications? Counters?

> +	}
> +
>  	nbp_switchdev_frame_mark(p, skb);
>  
>  	/* insert into forwarding database after filtering to avoid spoofing */
> -	br = p->br;
>  	if (p->flags & BR_LEARNING)
>  		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
>  
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 2ff83d84230d..7d4432ca9a20 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -184,6 +184,7 @@ static inline size_t br_port_info_size(void)
>  		+ nla_total_size(1)	/* IFLA_BRPORT_VLAN_TUNNEL */
>  		+ nla_total_size(1)	/* IFLA_BRPORT_NEIGH_SUPPRESS */
>  		+ nla_total_size(1)	/* IFLA_BRPORT_ISOLATED */
> +		+ nla_total_size(1)	/* IFLA_BRPORT_LOCKED */
>  		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_ROOT_ID */
>  		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_BRIDGE_ID */
>  		+ nla_total_size(sizeof(u16))	/* IFLA_BRPORT_DESIGNATED_PORT */
> @@ -269,7 +270,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>  							  BR_MRP_LOST_CONT)) ||
>  	    nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
>  		       !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
> -	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)))
> +	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) ||
> +	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)))
>  		return -EMSGSIZE;
>  
>  	timerval = br_timer_value(&p->message_age_timer);
> @@ -827,6 +829,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
>  	[IFLA_BRPORT_GROUP_FWD_MASK] = { .type = NLA_U16 },
>  	[IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
>  	[IFLA_BRPORT_ISOLATED]	= { .type = NLA_U8 },
> +	[IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
>  	[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
>  	[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
>  };
> @@ -893,6 +896,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
>  	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
>  	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);
>  
>  	changed_mask = old_flags ^ p->flags;
>  
> -- 
> 2.30.2
> 

Re: [PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Andrew Lunn]

> > +	if (p->flags & BR_PORT_LOCKED) {
> > +		fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
> > +		if (!(fdb_entry && fdb_entry->dst == p))
> > +			goto drop;
> 
> I'm not familiar with 802.1X so I have some questions:

Me neither.

> 
> 1. Do we need to differentiate between no FDB entry and an FDB entry
> pointing to a different port than we expect?

And extending that question, a static vs a dynamic entry?

    Andrew

Re: [PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Hans Schultz]

On mån, feb 07, 2022 at 14:53, Andrew Lunn <andrew@lunn.ch> wrote:
>> > +	if (p->flags & BR_PORT_LOCKED) {
>> > +		fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
>> > +		if (!(fdb_entry && fdb_entry->dst == p))
>> > +			goto drop;
>> 
>> I'm not familiar with 802.1X so I have some questions:
>
> Me neither.
>
>> 
>> 1. Do we need to differentiate between no FDB entry and an FDB entry
>> pointing to a different port than we expect?
>
> And extending that question, a static vs a dynamic entry?
>
>     Andrew

The question is - if there is an fdb entry or not - for the specific client
mac address behind the locked port in the bridge associated with the
respective locked port and vlan taken into consideration.
Normally you would have learning disabled, or from a fresh start if a port
is locked, it will not learn on incoming from that port, so you need to
add the fdb entry from user-space. In the common case you will want to
use static entries and remember the master flag for the entry to go to
the bridge module.

Re: [PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Hans Schultz]

On mån, feb 07, 2022 at 12:49, Ido Schimmel <idosch@idosch.org> wrote:
> On Mon, Feb 07, 2022 at 11:07:39AM +0100, Hans Schultz wrote:
>> In a 802.1X scenario, clients connected to a bridge port shall not
>> be allowed to have traffic forwarded until fully authenticated.
>> A static fdb entry of the clients MAC address for the bridge port
>> unlocks the client and allows bidirectional communication.
>> 
>> This scenario is facilitated with setting the bridge port in locked
>> mode, which is also supported by various switchcore chipsets.
>> 
>> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
>> ---
>>  include/linux/if_bridge.h    |  1 +
>>  include/uapi/linux/if_link.h |  1 +
>>  net/bridge/br_input.c        | 10 +++++++++-
>>  net/bridge/br_netlink.c      |  6 +++++-
>>  4 files changed, 16 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index 509e18c7e740..3aae023a9353 100644
>> --- a/include/linux/if_bridge.h
>> +++ b/include/linux/if_bridge.h
>> @@ -58,6 +58,7 @@ struct br_ip_list {
>>  #define BR_MRP_LOST_CONT	BIT(18)
>>  #define BR_MRP_LOST_IN_CONT	BIT(19)
>>  #define BR_TX_FWD_OFFLOAD	BIT(20)
>> +#define BR_PORT_LOCKED		BIT(21)
>>  
>>  #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
>>  
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index 6218f93f5c1a..8fa2648fbc83 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -532,6 +532,7 @@ enum {
>>  	IFLA_BRPORT_GROUP_FWD_MASK,
>>  	IFLA_BRPORT_NEIGH_SUPPRESS,
>>  	IFLA_BRPORT_ISOLATED,
>> +	IFLA_BRPORT_LOCKED,
>
> Please add it at the end to avoid breaking uAPI
>

Shall do.

>>  	IFLA_BRPORT_BACKUP_PORT,
>>  	IFLA_BRPORT_MRP_RING_OPEN,
>>  	IFLA_BRPORT_MRP_IN_OPEN,
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index b50382f957c1..469e3adbce07 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -69,6 +69,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>>  	enum br_pkt_type pkt_type = BR_PKT_UNICAST;
>>  	struct net_bridge_fdb_entry *dst = NULL;
>> +	struct net_bridge_fdb_entry *fdb_entry;
>>  	struct net_bridge_mcast_port *pmctx;
>>  	struct net_bridge_mdb_entry *mdst;
>>  	bool local_rcv, mcast_hit = false;
>> @@ -81,6 +82,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  	if (!p || p->state == BR_STATE_DISABLED)
>>  		goto drop;
>>  
>> +	br = p->br;
>> +
>>  	brmctx = &p->br->multicast_ctx;
>>  	pmctx = &p->multicast_ctx;
>>  	state = p->state;
>> @@ -88,10 +91,15 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>  				&state, &vlan))
>>  		goto out;
>>  
>> +	if (p->flags & BR_PORT_LOCKED) {
>> +		fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
>> +		if (!(fdb_entry && fdb_entry->dst == p))
>> +			goto drop;
>
> I'm not familiar with 802.1X so I have some questions:
>
> 1. Do we need to differentiate between no FDB entry and an FDB entry
> pointing to a different port than we expect?
>
> 2. Does user space care about SAs that did not pass the check? That is,
> does it need to see notifications? Counters?
>

2. As of now there are no counters, notifications on a locked port.

>> +	}
>> +
>>  	nbp_switchdev_frame_mark(p, skb);
>>  
>>  	/* insert into forwarding database after filtering to avoid spoofing */
>> -	br = p->br;
>>  	if (p->flags & BR_LEARNING)
>>  		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
>>  
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 2ff83d84230d..7d4432ca9a20 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -184,6 +184,7 @@ static inline size_t br_port_info_size(void)
>>  		+ nla_total_size(1)	/* IFLA_BRPORT_VLAN_TUNNEL */
>>  		+ nla_total_size(1)	/* IFLA_BRPORT_NEIGH_SUPPRESS */
>>  		+ nla_total_size(1)	/* IFLA_BRPORT_ISOLATED */
>> +		+ nla_total_size(1)	/* IFLA_BRPORT_LOCKED */
>>  		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_ROOT_ID */
>>  		+ nla_total_size(sizeof(struct ifla_bridge_id))	/* IFLA_BRPORT_BRIDGE_ID */
>>  		+ nla_total_size(sizeof(u16))	/* IFLA_BRPORT_DESIGNATED_PORT */
>> @@ -269,7 +270,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>>  							  BR_MRP_LOST_CONT)) ||
>>  	    nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN,
>>  		       !!(p->flags & BR_MRP_LOST_IN_CONT)) ||
>> -	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)))
>> +	    nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) ||
>> +	    nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED)))
>>  		return -EMSGSIZE;
>>  
>>  	timerval = br_timer_value(&p->message_age_timer);
>> @@ -827,6 +829,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
>>  	[IFLA_BRPORT_GROUP_FWD_MASK] = { .type = NLA_U16 },
>>  	[IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 },
>>  	[IFLA_BRPORT_ISOLATED]	= { .type = NLA_U8 },
>> +	[IFLA_BRPORT_LOCKED] = { .type = NLA_U8 },
>>  	[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
>>  	[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
>>  };
>> @@ -893,6 +896,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[],
>>  	br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL);
>>  	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);
>>  
>>  	changed_mask = old_flags ^ p->flags;
>>  
>> -- 
>> 2.30.2
>> 

Re: [PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Stephen Hemminger]

On Mon,  7 Feb 2022 11:07:39 +0100
Hans Schultz <schultz.hans@gmail.com> wrote:

> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -532,6 +532,7 @@ enum {
>  	IFLA_BRPORT_GROUP_FWD_MASK,
>  	IFLA_BRPORT_NEIGH_SUPPRESS,
>  	IFLA_BRPORT_ISOLATED,
> +	IFLA_BRPORT_LOCKED,
>  	IFLA_BRPORT_BACKUP_PORT,
>  	IFLA_BRPORT_MRP_RING_OPEN,

Adding new element in middle of enum causes them to be renumbered.
That breaks the kernel ABI. Please add only at end.

Re: [PATCH net-next 1/4] net: bridge: Add support for bridge port in locked mode

[Stephen Hemminger]

On Mon,  7 Feb 2022 11:07:39 +0100
Hans Schultz <schultz.hans@gmail.com> wrote:

> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 6218f93f5c1a..8fa2648fbc83 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -532,6 +532,7 @@ enum {
>  	IFLA_BRPORT_GROUP_FWD_MASK,
>  	IFLA_BRPORT_NEIGH_SUPPRESS,
>  	IFLA_BRPORT_ISOLATED,
> +	IFLA_BRPORT_LOCKED,
>  	IFLA_BRPORT_BACKUP_PORT,
>  	IFLA_BRPORT_MRP_RING_OPEN,
>  	IFLA_BRPORT_MRP_IN_OPEN,

NAK
This is userspace API, adding a new value in enum in the middle
will reorder the numbers and break ABI.

[PATCH net-next 2/4] net: bridge: dsa: Add support for offloading of locked port flag

[Hans Schultz]

Various switchcores support setting ports in locked mode, so that
clients behind locked ports cannot send traffic through the port
unless a fdb entry is added with the clients MAC address.

Among the switchcores that support this feature is the Marvell
mv88e6xxx family.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 net/bridge/br_switchdev.c | 2 +-
 net/dsa/port.c            | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index f8fbaaa7c501..bf549fc22556 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -72,7 +72,7 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p,
 
 /* Flags that can be offloaded to hardware */
 #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \
-				  BR_MCAST_FLOOD | BR_BCAST_FLOOD)
+				  BR_MCAST_FLOOD | BR_BCAST_FLOOD | BR_PORT_LOCKED)
 
 int br_switchdev_set_port_flag(struct net_bridge_port *p,
 			       unsigned long flags,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index bd78192e0e47..01ed22ed74a1 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -176,7 +176,7 @@ static int dsa_port_inherit_brport_flags(struct dsa_port *dp,
 					 struct netlink_ext_ack *extack)
 {
 	const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
-				   BR_BCAST_FLOOD;
+				   BR_BCAST_FLOOD | BR_PORT_LOCKED;
 	struct net_device *brport_dev = dsa_port_to_bridge_port(dp);
 	int flag, err;
 
@@ -200,7 +200,7 @@ static void dsa_port_clear_brport_flags(struct dsa_port *dp)
 {
 	const unsigned long val = BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
 	const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
-				   BR_BCAST_FLOOD;
+				   BR_BCAST_FLOOD | BR_PORT_LOCKED;
 	int flag, err;
 
 	for_each_set_bit(flag, &mask, 32) {
-- 
2.30.2

Re: [PATCH net-next 2/4] net: bridge: dsa: Add support for offloading of locked port flag

[Nikolay Aleksandrov]

On 07/02/2022 12:07, Hans Schultz wrote:
> Various switchcores support setting ports in locked mode, so that
> clients behind locked ports cannot send traffic through the port
> unless a fdb entry is added with the clients MAC address.
> 
> Among the switchcores that support this feature is the Marvell
> mv88e6xxx family.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  net/bridge/br_switchdev.c | 2 +-
>  net/dsa/port.c            | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 

Hi,
For some reason I still haven't received patch 01 which adds this new flag,
so I'll comment on this patch first.

These should be 2 separate patches, there is no bridge: dsa:.
Please break them into a bridge: and dsa: parts that add the flag, it also
makes it easier for people who have to backport patches and fixes later.

In addition please mention in the commit message why the flag is
added there, i.e. in order to allow to be offloaded.

Thanks,
 Nik

[PATCH net-next 3/4] net: dsa: mv88e6xxx: Add support for bridge port locked feature

[Hans Schultz]

Supporting bridge port locked mode using the 802.1X mode in Marvell
mv88e6xxx switchcores is described in the '88E6096/88E6097/88E6097F
Datasheet', sections 4.4.6, 4.4.7 and 5.1.2.1 (Drop on Lock).

This feature is implemented here facilitated by the locked port flag.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c |  9 ++++++++-
 drivers/net/dsa/mv88e6xxx/port.c | 33 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.h |  3 +++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 58ca684d73f7..eed3713b97ae 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5881,7 +5881,7 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 	const struct mv88e6xxx_ops *ops;
 
 	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
-			   BR_BCAST_FLOOD))
+			   BR_BCAST_FLOOD | BR_PORT_LOCKED))
 		return -EINVAL;
 
 	ops = chip->info->ops;
@@ -5939,6 +5939,13 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 			goto out;
 	}
 
+	if (flags.mask & BR_PORT_LOCKED) {
+		bool locked = !!(flags.val & BR_PORT_LOCKED);
+
+		err = mv88e6xxx_port_set_lock(chip, port, locked);
+		if (err)
+			goto out;
+	}
 out:
 	mv88e6xxx_reg_unlock(chip);
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index ab41619a809b..2279936429f9 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1234,6 +1234,39 @@ int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
 	return err;
 }
 
+int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
+			    bool locked)
+{
+	u16 reg;
+	int err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
+	if (err)
+		return err;
+
+	reg &= ~MV88E6XXX_PORT_CTL0_DROP_ON_LOCK;
+	if (locked)
+		reg |= MV88E6XXX_PORT_CTL0_DROP_ON_LOCK;
+
+	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, &reg);
+	if (err)
+		return err;
+
+	reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+	if (locked)
+		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+
+	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,
 				  u16 mode)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 03382b66f800..655d942ac657 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -365,6 +365,9 @@ int mv88e6xxx_port_set_fid(struct mv88e6xxx_chip *chip, int port, u16 fid);
 int mv88e6xxx_port_get_pvid(struct mv88e6xxx_chip *chip, int port, u16 *pvid);
 int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, u16 pvid);
 
+int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
+			    bool locked);
+
 int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,
 				  u16 mode);
 int mv88e6095_port_tag_remap(struct mv88e6xxx_chip *chip, int port);
-- 
2.30.2

Re: [PATCH net-next 3/4] net: dsa: mv88e6xxx: Add support for bridge port locked feature

[Andrew Lunn]

On Mon, Feb 07, 2022 at 11:07:41AM +0100, Hans Schultz wrote:
> Supporting bridge port locked mode using the 802.1X mode in Marvell
> mv88e6xxx switchcores is described in the '88E6096/88E6097/88E6097F
> Datasheet', sections 4.4.6, 4.4.7 and 5.1.2.1 (Drop on Lock).

This implementation seems to be incorrect for 6390X, and maybe
others. I just picked a modern devices at random, and it is different,
so didn't check any other devices.  The 6390X uses bits 14 and 15, not
just bit 14.

So either you need to narrow down support to just those devices this
actually works for, or you need to add implementations for all
generations, via an op in mv88e6xxx_ops.

    Andrew

Re: [PATCH net-next 3/4] net: dsa: mv88e6xxx: Add support for bridge port locked feature

[Hans Schultz]

On mån, feb 07, 2022 at 15:05, Andrew Lunn <andrew@lunn.ch> wrote:
> On Mon, Feb 07, 2022 at 11:07:41AM +0100, Hans Schultz wrote:
>> Supporting bridge port locked mode using the 802.1X mode in Marvell
>> mv88e6xxx switchcores is described in the '88E6096/88E6097/88E6097F
>> Datasheet', sections 4.4.6, 4.4.7 and 5.1.2.1 (Drop on Lock).
>
> This implementation seems to be incorrect for 6390X, and maybe
> others. I just picked a modern devices at random, and it is different,
> so didn't check any other devices.  The 6390X uses bits 14 and 15, not
> just bit 14.
>
> So either you need to narrow down support to just those devices this
> actually works for, or you need to add implementations for all
> generations, via an op in mv88e6xxx_ops.
>
>     Andrew

The 6096 and 6097 also use both bits 15 and 14, with '01' being Drop On
Lock and the default being '00' No SA filtering. '11' is drop to CPU, which
can also be used for 801.1X, so 'x1' should suffice for these devices,
thus setting bit 14 seems appropriate.

Re: [PATCH net-next 3/4] net: dsa: mv88e6xxx: Add support for bridge port locked feature

[Andrew Lunn]

On Tue, Feb 08, 2022 at 01:14:45PM +0100, Hans Schultz wrote:
> On mån, feb 07, 2022 at 15:05, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Mon, Feb 07, 2022 at 11:07:41AM +0100, Hans Schultz wrote:
> >> Supporting bridge port locked mode using the 802.1X mode in Marvell
> >> mv88e6xxx switchcores is described in the '88E6096/88E6097/88E6097F
> >> Datasheet', sections 4.4.6, 4.4.7 and 5.1.2.1 (Drop on Lock).
> >
> > This implementation seems to be incorrect for 6390X, and maybe
> > others. I just picked a modern devices at random, and it is different,
> > so didn't check any other devices.  The 6390X uses bits 14 and 15, not
> > just bit 14.
> >
> > So either you need to narrow down support to just those devices this
> > actually works for, or you need to add implementations for all
> > generations, via an op in mv88e6xxx_ops.
> >
> >     Andrew
> 
> The 6096 and 6097 also use both bits 15 and 14, with '01' being Drop On
> Lock and the default being '00' No SA filtering. '11' is drop to CPU, which
> can also be used for 801.1X, so 'x1' should suffice for these devices,
> thus setting bit 14 seems appropriate.

Your code does not make this clear. Please define all four values, and
then mask and set both bits as needed.

Thanks

    Andrew

[PATCH net-next 4/4] net: bridge: Refactor bridge port in locked mode to use jump labels

[Hans Schultz]

As the locked mode feature is in the hot path of the bridge modules
reception of packets, it needs to be refactored to use jump labels
for optimization.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 net/bridge/br_input.c   | 22 ++++++++++++++++++----
 net/bridge/br_netlink.c |  6 ++++++
 net/bridge/br_private.h |  2 ++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 469e3adbce07..6fc428d6bac5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -23,6 +23,18 @@
 #include "br_private.h"
 #include "br_private_tunnel.h"
 
+static struct static_key_false br_input_locked_port_feature;
+
+void br_input_locked_port_add(void)
+{
+	static_branch_inc(&br_input_locked_port_feature);
+}
+
+void br_input_locked_port_remove(void)
+{
+	static_branch_dec(&br_input_locked_port_feature);
+}
+
 static int
 br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
@@ -91,10 +103,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 				&state, &vlan))
 		goto out;
 
-	if (p->flags & BR_PORT_LOCKED) {
-		fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
-		if (!(fdb_entry && fdb_entry->dst == p))
-			goto drop;
+	if (static_branch_unlikely(&br_input_locked_port_feature)) {
+		if (p->flags & BR_PORT_LOCKED) {
+			fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
+			if (!(fdb_entry && fdb_entry->dst == p))
+				goto drop;
+		}
 	}
 
 	nbp_switchdev_frame_mark(p, skb);
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 7d4432ca9a20..e3dbe9fed75c 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -860,6 +860,7 @@ static int br_set_port_state(struct net_bridge_port *p, u8 state)
 static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 			     int attrtype, unsigned long mask)
 {
+	bool locked = p->flags & BR_PORT_LOCKED;
 	if (!tb[attrtype])
 		return;
 
@@ -867,6 +868,11 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 		p->flags |= mask;
 	else
 		p->flags &= ~mask;
+
+	if ((p->flags & BR_PORT_LOCKED) && !locked)
+		br_input_locked_port_add();
+	if (!(p->flags & BR_PORT_LOCKED) && locked)
+		br_input_locked_port_remove();
 }
 
 /* Process bridge protocol info on port */
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 2661dda1a92b..0ec3ef897978 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -832,6 +832,8 @@ void br_manage_promisc(struct net_bridge *br);
 int nbp_backup_change(struct net_bridge_port *p, struct net_device *backup_dev);
 
 /* br_input.c */
+void br_input_locked_port_add(void);
+void br_input_locked_port_remove(void);
 int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
 rx_handler_func_t *br_get_rx_handler(const struct net_device *dev);
 
-- 
2.30.2