[PATCH net-next v3 0/5] Add support for locked bridge ports (for 802.1X)

[PATCH net-next v3 0/5] Add support for locked bridge ports (for 802.1X)

[Hans Schultz]

This series starts by adding support for SA filtering to the bridge,
which is then allowed to be offloaded to switchdev devices. Furthermore
an offloading implementation is supplied for the mv88e6xxx driver.

Public Local Area Networks are often deployed such that there is a
risk of unauthorized or unattended clients getting access to the LAN.
To prevent such access we introduce SA filtering, such that ports
designated as secure ports are set in locked mode, so that only
authorized source MAC addresses are given access by adding them to
the bridges forwarding database. Incoming packets with source MAC
addresses that are not in the forwarding database of the bridge are
discarded. It is then the task of user space daemons to populate the
bridge's forwarding database with static entries of authorized entities.

The most common approach is to use the IEEE 802.1X protocol to take
care of the authorization of allowed users to gain access by opening
for the source address of the authorized host.

With the current use of the bridge parameter in hostapd, there is
a limitation in using this for IEEE 802.1X port authentication. It
depends on hostapd attaching the port on which it has a successful
authentication to the bridge, but that only allows for a single
authentication per port. This patch set allows for the use of
IEEE 802.1X port authentication in a more general network context with
multiple 802.1X aware hosts behind a single port as depicted, which is
a commonly used commercial use-case, as it is only the number of
available entries in the forwarding database that limits the number of
authenticated clients.

      +--------------------------------+
      |                                |
      |      Bridge/Authenticator      |
      |                                |
      +-------------+------------------+
       802.1X port  |
                    |
                    |
             +------+-------+
             |              |
             |  Hub/Switch  |
             |              |
             +-+----------+-+
               |          |
            +--+--+    +--+--+
            |     |    |     |
    Hosts   |  a  |    |  b  |   . . .
            |     |    |     |
            +-----+    +-----+

The 802.1X standard involves three different components, a Supplicant
(Host), an Authenticator (Network Access Point) and an Authentication
Server which is typically a Radius server. This patch set thus enables
the bridge module together with an authenticator application to serve
as an Authenticator on designated ports.


For the bridge to become an IEEE 802.1X Authenticator, a solution using
hostapd with the bridge driver can be found at
https://github.com/westermo/hostapd/tree/bridge_driver .


The relevant components work transparently in relation to if it is the
bridge module or the offloaded switchcore case that is in use.


Hans Schultz (5):
  net: bridge: Add support for bridge port in locked mode
  net: bridge: Add support for offloading of locked port flag
  net: dsa: Add support for offloaded locked port flag
  net: dsa: mv88e6xxx: Add support for bridge port locked mode
  selftests: forwarding: tests of locked port feature

 drivers/net/dsa/mv88e6xxx/chip.c              |   9 +-
 drivers/net/dsa/mv88e6xxx/port.c              |  33 ++++
 drivers/net/dsa/mv88e6xxx/port.h              |   9 +-
 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 +-
 net/bridge/br_switchdev.c                     |   2 +-
 net/dsa/port.c                                |   4 +-
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
 tools/testing/selftests/net/forwarding/lib.sh |  16 ++
 12 files changed, 259 insertions(+), 7 deletions(-)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh

-- 
2.30.2

[Bridge] [PATCH net-next v3 0/5] Add support for locked bridge ports (for 802.1X)

[Hans Schultz]

This series starts by adding support for SA filtering to the bridge,
which is then allowed to be offloaded to switchdev devices. Furthermore
an offloading implementation is supplied for the mv88e6xxx driver.

Public Local Area Networks are often deployed such that there is a
risk of unauthorized or unattended clients getting access to the LAN.
To prevent such access we introduce SA filtering, such that ports
designated as secure ports are set in locked mode, so that only
authorized source MAC addresses are given access by adding them to
the bridges forwarding database. Incoming packets with source MAC
addresses that are not in the forwarding database of the bridge are
discarded. It is then the task of user space daemons to populate the
bridge's forwarding database with static entries of authorized entities.

The most common approach is to use the IEEE 802.1X protocol to take
care of the authorization of allowed users to gain access by opening
for the source address of the authorized host.

With the current use of the bridge parameter in hostapd, there is
a limitation in using this for IEEE 802.1X port authentication. It
depends on hostapd attaching the port on which it has a successful
authentication to the bridge, but that only allows for a single
authentication per port. This patch set allows for the use of
IEEE 802.1X port authentication in a more general network context with
multiple 802.1X aware hosts behind a single port as depicted, which is
a commonly used commercial use-case, as it is only the number of
available entries in the forwarding database that limits the number of
authenticated clients.

      +--------------------------------+
      |                                |
      |      Bridge/Authenticator      |
      |                                |
      +-------------+------------------+
       802.1X port  |
                    |
                    |
             +------+-------+
             |              |
             |  Hub/Switch  |
             |              |
             +-+----------+-+
               |          |
            +--+--+    +--+--+
            |     |    |     |
    Hosts   |  a  |    |  b  |   . . .
            |     |    |     |
            +-----+    +-----+

The 802.1X standard involves three different components, a Supplicant
(Host), an Authenticator (Network Access Point) and an Authentication
Server which is typically a Radius server. This patch set thus enables
the bridge module together with an authenticator application to serve
as an Authenticator on designated ports.


For the bridge to become an IEEE 802.1X Authenticator, a solution using
hostapd with the bridge driver can be found at
https://github.com/westermo/hostapd/tree/bridge_driver .


The relevant components work transparently in relation to if it is the
bridge module or the offloaded switchcore case that is in use.


Hans Schultz (5):
  net: bridge: Add support for bridge port in locked mode
  net: bridge: Add support for offloading of locked port flag
  net: dsa: Add support for offloaded locked port flag
  net: dsa: mv88e6xxx: Add support for bridge port locked mode
  selftests: forwarding: tests of locked port feature

 drivers/net/dsa/mv88e6xxx/chip.c              |   9 +-
 drivers/net/dsa/mv88e6xxx/port.c              |  33 ++++
 drivers/net/dsa/mv88e6xxx/port.h              |   9 +-
 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 +-
 net/bridge/br_switchdev.c                     |   2 +-
 net/dsa/port.c                                |   4 +-
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
 tools/testing/selftests/net/forwarding/lib.sh |  16 ++
 12 files changed, 259 insertions(+), 7 deletions(-)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh

-- 
2.30.2

[PATCH net-next v3 1/5] 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..a45cc0a1f415 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -537,6 +537,7 @@ enum {
 	IFLA_BRPORT_MRP_IN_OPEN,
 	IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
 	IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
+	IFLA_BRPORT_LOCKED,
 	__IFLA_BRPORT_MAX
 };
 #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index b50382f957c1..e99f635ff727 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -81,6 +81,7 @@ 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 +89,17 @@ 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) {
+		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))
+			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

[Bridge] [PATCH net-next v3 1/5] 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..a45cc0a1f415 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -537,6 +537,7 @@ enum {
 	IFLA_BRPORT_MRP_IN_OPEN,
 	IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
 	IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
+	IFLA_BRPORT_LOCKED,
 	__IFLA_BRPORT_MAX
 };
 #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index b50382f957c1..e99f635ff727 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -81,6 +81,7 @@ 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 +89,17 @@ 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) {
+		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))
+			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

[PATCH net-next v3 2/5] net: bridge: 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.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 net/bridge/br_switchdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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,
-- 
2.30.2

[Bridge] [PATCH net-next v3 2/5] net: bridge: 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.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 net/bridge/br_switchdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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,
-- 
2.30.2

[PATCH net-next v3 3/5] net: dsa: Add support for offloaded locked port flag

[Hans Schultz]

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

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

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

[Bridge] [PATCH net-next v3 3/5] net: dsa: Add support for offloaded locked port flag

[Hans Schultz]

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

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

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

[PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Hans Schultz]

Supporting bridge ports in locked mode using the drop on lock
feature 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 |  9 ++++++++-
 3 files changed, 49 insertions(+), 2 deletions(-)

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..46b7381899a0 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_SA_FILT_MASK;
+	if (locked)
+		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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..3f70557f0d48 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -142,7 +142,11 @@
 /* Offset 0x04: Port Control Register */
 #define MV88E6XXX_PORT_CTL0					0x04
 #define MV88E6XXX_PORT_CTL0_USE_CORE_TAG			0x8000
-#define MV88E6XXX_PORT_CTL0_DROP_ON_LOCK			0x4000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_MASK			0xc000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DISABLED			0x0000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK		0x4000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_UNLOCK		0x8000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_CPU		0xc000
 #define MV88E6XXX_PORT_CTL0_EGRESS_MODE_MASK			0x3000
 #define MV88E6XXX_PORT_CTL0_EGRESS_MODE_UNMODIFIED		0x0000
 #define MV88E6XXX_PORT_CTL0_EGRESS_MODE_UNTAGGED		0x1000
@@ -365,6 +369,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

[Bridge] [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Hans Schultz]

Supporting bridge ports in locked mode using the drop on lock
feature 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 |  9 ++++++++-
 3 files changed, 49 insertions(+), 2 deletions(-)

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..46b7381899a0 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_SA_FILT_MASK;
+	if (locked)
+		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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..3f70557f0d48 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -142,7 +142,11 @@
 /* Offset 0x04: Port Control Register */
 #define MV88E6XXX_PORT_CTL0					0x04
 #define MV88E6XXX_PORT_CTL0_USE_CORE_TAG			0x8000
-#define MV88E6XXX_PORT_CTL0_DROP_ON_LOCK			0x4000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_MASK			0xc000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DISABLED			0x0000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK		0x4000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_UNLOCK		0x8000
+#define MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_CPU		0xc000
 #define MV88E6XXX_PORT_CTL0_EGRESS_MODE_MASK			0x3000
 #define MV88E6XXX_PORT_CTL0_EGRESS_MODE_UNMODIFIED		0x0000
 #define MV88E6XXX_PORT_CTL0_EGRESS_MODE_UNTAGGED		0x1000
@@ -365,6 +369,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

[PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Hans Schultz]

These tests check that the basic locked port feature works, so that no 'host'
can communicate (ping) through a locked port unless the MAC address of the
'host' interface is in the forwarding database of the bridge.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
 tools/testing/selftests/net/forwarding/lib.sh |  16 ++
 3 files changed, 191 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 72ee644d47bf..8fa97ae9af9e 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+ OR MIT
 
 TEST_PROGS = bridge_igmp.sh \
+	bridge_locked_port.sh \
 	bridge_port_isolation.sh \
 	bridge_sticky_fdb.sh \
 	bridge_vlan_aware.sh \
diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
new file mode 100755
index 000000000000..d2805441b325
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -0,0 +1,174 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
+NUM_NETIFS=4
+CHECK_TC="no"
+source lib.sh
+
+h1_create()
+{
+	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
+	vrf_create "vrf-vlan-h1"
+        ip link set dev vrf-vlan-h1 up
+        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64
+}
+
+h1_destroy()
+{
+	vlan_destroy $h1 100
+	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
+}
+
+h2_create()
+{
+	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
+	vrf_create "vrf-vlan-h2"
+	ip link set dev vrf-vlan-h2 up
+	vlan_create $h2 100 vrf-vlan-h2 192.0.3.2/24 2001:db8:3::2/64
+}
+
+h2_destroy()
+{
+	vlan_destroy $h2 100
+	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
+}
+
+switch_create()
+{
+	ip link add dev br0 type bridge vlan_filtering 1
+
+	ip link set dev $swp1 master br0
+	ip link set dev $swp2 master br0
+
+	ip link set dev br0 up
+	ip link set dev $swp1 up
+	ip link set dev $swp2 up
+
+	bridge link set dev $swp1 learning off
+}
+
+switch_destroy()
+{
+	ip link set dev $swp2 down
+	ip link set dev $swp1 down
+
+	ip link del dev br0
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	swp1=${NETIFS[p2]}
+
+	swp2=${NETIFS[p3]}
+	h2=${NETIFS[p4]}
+
+	vrf_prepare
+
+	h1_create
+	h2_create
+
+	switch_create
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	switch_destroy
+
+	h2_destroy
+	h1_destroy
+
+	vrf_cleanup
+}
+
+ifaddr()
+{
+	ip -br link show dev "$1" | awk '{ print($3); }'
+}
+
+locked_port_ipv4()
+{
+	RET=0
+
+	check_locked_port_support || return 0
+
+	ping_do $h1 192.0.2.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked on
+
+	ping_do $h1 192.0.2.2
+	check_fail $? "Ping worked when it should not have"
+
+	bridge fdb add `ifaddr $h1` dev $swp1 master static
+
+	ping_do $h1 192.0.2.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked off
+	bridge fdb del `ifaddr $h1` dev $swp1 master static
+	log_test "Locked port ipv4"
+}
+
+locked_port_vlan()
+{
+	RET=0
+
+	check_locked_port_support || return 0
+	check_vlan_filtering_support || return 0
+
+	bridge vlan add vid 100 dev $swp1 tagged
+	bridge vlan add vid 100 dev $swp2 tagged
+
+	ping_do $h1.100 192.0.3.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked on
+	ping_do $h1.100 192.0.3.2
+	check_fail $? "Ping worked when it should not have"
+
+	bridge fdb add `ifaddr $h1` dev $swp1 vlan 100 master static
+
+	ping_do $h1.100 192.0.3.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked off
+	bridge vlan del vid 100 dev $swp1
+	bridge vlan del vid 100 dev $swp2
+	bridge fdb del `ifaddr $h1` dev $swp1 vlan 100 master static
+	log_test "Locked port vlan"
+}
+
+locked_port_ipv6()
+{
+	RET=0
+	check_locked_port_support || return 0
+
+	ping6_do $h1 2001:db8:1::2
+	check_err $? "Ping6 didn't work when it should have"
+
+	bridge link set dev $swp1 locked on
+
+	ping6_do $h1 2001:db8:1::2
+	check_fail $? "Ping worked when it should not have"
+
+	bridge fdb add `ifaddr $h1` dev $swp1 master static
+	ping6_do $h1 2001:db8:1::2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked off
+	bridge fdb del `ifaddr $h1` dev $swp1 master static
+	log_test "Locked port ipv6"
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 7da783d6f453..9ded90f17ead 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -125,6 +125,22 @@ check_ethtool_lanes_support()
 	fi
 }
 
+check_locked_port_support()
+{
+        if ! bridge -d link show | grep -q " locked"; then
+                echo "SKIP: iproute2 too old; Locked port feature not supported."
+                return $ksft_skip
+        fi
+}
+
+check_vlan_filtering_support()
+{
+	if ! bridge -d vlan show | grep -q "state forwarding"; then
+		echo "SKIP: vlan filtering not supported."
+		return $ksft_skip
+	fi
+}
+
 if [[ "$(id -u)" -ne 0 ]]; then
 	echo "SKIP: need root privileges"
 	exit $ksft_skip
-- 
2.30.2

[Bridge] [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Hans Schultz]

These tests check that the basic locked port feature works, so that no 'host'
can communicate (ping) through a locked port unless the MAC address of the
'host' interface is in the forwarding database of the bridge.

Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
---
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
 tools/testing/selftests/net/forwarding/lib.sh |  16 ++
 3 files changed, 191 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 72ee644d47bf..8fa97ae9af9e 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+ OR MIT
 
 TEST_PROGS = bridge_igmp.sh \
+	bridge_locked_port.sh \
 	bridge_port_isolation.sh \
 	bridge_sticky_fdb.sh \
 	bridge_vlan_aware.sh \
diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
new file mode 100755
index 000000000000..d2805441b325
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -0,0 +1,174 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
+NUM_NETIFS=4
+CHECK_TC="no"
+source lib.sh
+
+h1_create()
+{
+	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
+	vrf_create "vrf-vlan-h1"
+        ip link set dev vrf-vlan-h1 up
+        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64
+}
+
+h1_destroy()
+{
+	vlan_destroy $h1 100
+	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
+}
+
+h2_create()
+{
+	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
+	vrf_create "vrf-vlan-h2"
+	ip link set dev vrf-vlan-h2 up
+	vlan_create $h2 100 vrf-vlan-h2 192.0.3.2/24 2001:db8:3::2/64
+}
+
+h2_destroy()
+{
+	vlan_destroy $h2 100
+	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
+}
+
+switch_create()
+{
+	ip link add dev br0 type bridge vlan_filtering 1
+
+	ip link set dev $swp1 master br0
+	ip link set dev $swp2 master br0
+
+	ip link set dev br0 up
+	ip link set dev $swp1 up
+	ip link set dev $swp2 up
+
+	bridge link set dev $swp1 learning off
+}
+
+switch_destroy()
+{
+	ip link set dev $swp2 down
+	ip link set dev $swp1 down
+
+	ip link del dev br0
+}
+
+setup_prepare()
+{
+	h1=${NETIFS[p1]}
+	swp1=${NETIFS[p2]}
+
+	swp2=${NETIFS[p3]}
+	h2=${NETIFS[p4]}
+
+	vrf_prepare
+
+	h1_create
+	h2_create
+
+	switch_create
+}
+
+cleanup()
+{
+	pre_cleanup
+
+	switch_destroy
+
+	h2_destroy
+	h1_destroy
+
+	vrf_cleanup
+}
+
+ifaddr()
+{
+	ip -br link show dev "$1" | awk '{ print($3); }'
+}
+
+locked_port_ipv4()
+{
+	RET=0
+
+	check_locked_port_support || return 0
+
+	ping_do $h1 192.0.2.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked on
+
+	ping_do $h1 192.0.2.2
+	check_fail $? "Ping worked when it should not have"
+
+	bridge fdb add `ifaddr $h1` dev $swp1 master static
+
+	ping_do $h1 192.0.2.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked off
+	bridge fdb del `ifaddr $h1` dev $swp1 master static
+	log_test "Locked port ipv4"
+}
+
+locked_port_vlan()
+{
+	RET=0
+
+	check_locked_port_support || return 0
+	check_vlan_filtering_support || return 0
+
+	bridge vlan add vid 100 dev $swp1 tagged
+	bridge vlan add vid 100 dev $swp2 tagged
+
+	ping_do $h1.100 192.0.3.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked on
+	ping_do $h1.100 192.0.3.2
+	check_fail $? "Ping worked when it should not have"
+
+	bridge fdb add `ifaddr $h1` dev $swp1 vlan 100 master static
+
+	ping_do $h1.100 192.0.3.2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked off
+	bridge vlan del vid 100 dev $swp1
+	bridge vlan del vid 100 dev $swp2
+	bridge fdb del `ifaddr $h1` dev $swp1 vlan 100 master static
+	log_test "Locked port vlan"
+}
+
+locked_port_ipv6()
+{
+	RET=0
+	check_locked_port_support || return 0
+
+	ping6_do $h1 2001:db8:1::2
+	check_err $? "Ping6 didn't work when it should have"
+
+	bridge link set dev $swp1 locked on
+
+	ping6_do $h1 2001:db8:1::2
+	check_fail $? "Ping worked when it should not have"
+
+	bridge fdb add `ifaddr $h1` dev $swp1 master static
+	ping6_do $h1 2001:db8:1::2
+	check_err $? "Ping didn't work when it should have"
+
+	bridge link set dev $swp1 locked off
+	bridge fdb del `ifaddr $h1` dev $swp1 master static
+	log_test "Locked port ipv6"
+}
+
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 7da783d6f453..9ded90f17ead 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -125,6 +125,22 @@ check_ethtool_lanes_support()
 	fi
 }
 
+check_locked_port_support()
+{
+        if ! bridge -d link show | grep -q " locked"; then
+                echo "SKIP: iproute2 too old; Locked port feature not supported."
+                return $ksft_skip
+        fi
+}
+
+check_vlan_filtering_support()
+{
+	if ! bridge -d vlan show | grep -q "state forwarding"; then
+		echo "SKIP: vlan filtering not supported."
+		return $ksft_skip
+	fi
+}
+
 if [[ "$(id -u)" -ne 0 ]]; then
 	echo "SKIP: need root privileges"
 	exit $ksft_skip
-- 
2.30.2

Re: [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Shuah Khan]

On 2/18/22 8:51 AM, Hans Schultz wrote:
> These tests check that the basic locked port feature works, so that no 'host'
> can communicate (ping) through a locked port unless the MAC address of the
> 'host' interface is in the forwarding database of the bridge.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>   .../testing/selftests/net/forwarding/Makefile |   1 +
>   .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>   tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>   3 files changed, 191 insertions(+)
>   create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index 72ee644d47bf..8fa97ae9af9e 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0+ OR MIT
>   
>   TEST_PROGS = bridge_igmp.sh \
> +	bridge_locked_port.sh \
>   	bridge_port_isolation.sh \
>   	bridge_sticky_fdb.sh \
>   	bridge_vlan_aware.sh \

Looks good to me. Looks like TEST_PROGS # is getting close to 60.
Cool.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [Bridge] [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Shuah Khan]

On 2/18/22 8:51 AM, Hans Schultz wrote:
> These tests check that the basic locked port feature works, so that no 'host'
> can communicate (ping) through a locked port unless the MAC address of the
> 'host' interface is in the forwarding database of the bridge.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>   .../testing/selftests/net/forwarding/Makefile |   1 +
>   .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>   tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>   3 files changed, 191 insertions(+)
>   create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index 72ee644d47bf..8fa97ae9af9e 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0+ OR MIT
>   
>   TEST_PROGS = bridge_igmp.sh \
> +	bridge_locked_port.sh \
>   	bridge_port_isolation.sh \
>   	bridge_sticky_fdb.sh \
>   	bridge_vlan_aware.sh \

Looks good to me. Looks like TEST_PROGS # is getting close to 60.
Cool.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

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

[Nikolay Aleksandrov]

On 18/02/2022 17:51, 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(-)
> 

Hi Hans,
The patch looks good overall, I have one minor cosmetic comment below.

> 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..a45cc0a1f415 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -537,6 +537,7 @@ enum {
>  	IFLA_BRPORT_MRP_IN_OPEN,
>  	IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
>  	IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
> +	IFLA_BRPORT_LOCKED,
>  	__IFLA_BRPORT_MAX
>  };
>  #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index b50382f957c1..e99f635ff727 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -81,6 +81,7 @@ 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 +89,17 @@ 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) {
> +		struct net_bridge_fdb_entry *fdb_src =
> +			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);

Please leave an empty line between variable declaration and the code.

> +		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> +		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> +			goto drop;
> +	}
> +

With the above change you can add my Acked-by tag.

Thanks,
 Nik

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

[Nikolay Aleksandrov]

On 18/02/2022 17:51, 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(-)
> 

Hi Hans,
The patch looks good overall, I have one minor cosmetic comment below.

> 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..a45cc0a1f415 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -537,6 +537,7 @@ enum {
>  	IFLA_BRPORT_MRP_IN_OPEN,
>  	IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
>  	IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
> +	IFLA_BRPORT_LOCKED,
>  	__IFLA_BRPORT_MAX
>  };
>  #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index b50382f957c1..e99f635ff727 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -81,6 +81,7 @@ 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 +89,17 @@ 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) {
> +		struct net_bridge_fdb_entry *fdb_src =
> +			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);

Please leave an empty line between variable declaration and the code.

> +		if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
> +		    test_bit(BR_FDB_LOCAL, &fdb_src->flags))
> +			goto drop;
> +	}
> +

With the above change you can add my Acked-by tag.

Thanks,
 Nik

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

[Nikolay Aleksandrov]

On 18/02/2022 17:51, 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.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  net/bridge/br_switchdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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,

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

Re: [Bridge] [PATCH net-next v3 2/5] net: bridge: Add support for offloading of locked port flag

[Nikolay Aleksandrov]

On 18/02/2022 17:51, 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.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  net/bridge/br_switchdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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,

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

Re: [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Nikolay Aleksandrov]

On 18/02/2022 17:51, Hans Schultz wrote:
> These tests check that the basic locked port feature works, so that no 'host'
> can communicate (ping) through a locked port unless the MAC address of the
> 'host' interface is in the forwarding database of the bridge.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  .../testing/selftests/net/forwarding/Makefile |   1 +
>  .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>  3 files changed, 191 insertions(+)
>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index 72ee644d47bf..8fa97ae9af9e 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>  
>  TEST_PROGS = bridge_igmp.sh \
> +	bridge_locked_port.sh \
>  	bridge_port_isolation.sh \
>  	bridge_sticky_fdb.sh \
>  	bridge_vlan_aware.sh \
> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> new file mode 100755
> index 000000000000..d2805441b325
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> @@ -0,0 +1,174 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
> +NUM_NETIFS=4
> +CHECK_TC="no"
> +source lib.sh
> +
> +h1_create()
> +{
> +	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
> +	vrf_create "vrf-vlan-h1"
> +        ip link set dev vrf-vlan-h1 up
> +        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64
> +}

Please use tabs similar to everywhere else in the file.

Re: [Bridge] [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Nikolay Aleksandrov]

On 18/02/2022 17:51, Hans Schultz wrote:
> These tests check that the basic locked port feature works, so that no 'host'
> can communicate (ping) through a locked port unless the MAC address of the
> 'host' interface is in the forwarding database of the bridge.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  .../testing/selftests/net/forwarding/Makefile |   1 +
>  .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>  3 files changed, 191 insertions(+)
>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index 72ee644d47bf..8fa97ae9af9e 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>  
>  TEST_PROGS = bridge_igmp.sh \
> +	bridge_locked_port.sh \
>  	bridge_port_isolation.sh \
>  	bridge_sticky_fdb.sh \
>  	bridge_vlan_aware.sh \
> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> new file mode 100755
> index 000000000000..d2805441b325
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> @@ -0,0 +1,174 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
> +NUM_NETIFS=4
> +CHECK_TC="no"
> +source lib.sh
> +
> +h1_create()
> +{
> +	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
> +	vrf_create "vrf-vlan-h1"
> +        ip link set dev vrf-vlan-h1 up
> +        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64
> +}

Please use tabs similar to everywhere else in the file.

Re: [PATCH net-next v3 3/5] net: dsa: Add support for offloaded locked port flag

[Vladimir Oltean]

On Fri, Feb 18, 2022 at 04:51:46PM +0100, Hans Schultz wrote:
> Among the switchcores that support this feature is the Marvell
> mv88e6xxx family.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Although this doesn't "add support for offloaded locked ports", that
passes right through with no DSA-level filtering, from
SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS to dsa_port_bridge_flags(),
to ds->ops->port_bridge_flags().

Rather, a clearer description of what this does is:

net: dsa: include BR_PORT_LOCKED in the list of synced brport flags

Make sure the DSA switch driver gets notified of changes to the
BR_PORT_LOCKED flag as well, for the case when a DSA port joins or
leaves a LAG that is a bridge port.

Re: [Bridge] [PATCH net-next v3 3/5] net: dsa: Add support for offloaded locked port flag

[Vladimir Oltean]

On Fri, Feb 18, 2022 at 04:51:46PM +0100, Hans Schultz wrote:
> Among the switchcores that support this feature is the Marvell
> mv88e6xxx family.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Although this doesn't "add support for offloaded locked ports", that
passes right through with no DSA-level filtering, from
SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS to dsa_port_bridge_flags(),
to ds->ops->port_bridge_flags().

Rather, a clearer description of what this does is:

net: dsa: include BR_PORT_LOCKED in the list of synced brport flags

Make sure the DSA switch driver gets notified of changes to the
BR_PORT_LOCKED flag as well, for the case when a DSA port joins or
leaves a LAG that is a bridge port.

Re: [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Vladimir Oltean]

On Fri, Feb 18, 2022 at 04:51:47PM +0100, Hans Schultz wrote:
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ab41619a809b..46b7381899a0 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_SA_FILT_MASK;
> +	if (locked)
> +		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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);

	return mv88e6xxx_port_write(...);

> +	if (err)
> +		return err;
> +
> +	return 0;
> +}

Re: [Bridge] [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Vladimir Oltean]

On Fri, Feb 18, 2022 at 04:51:47PM +0100, Hans Schultz wrote:
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ab41619a809b..46b7381899a0 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_SA_FILT_MASK;
> +	if (locked)
> +		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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);

	return mv88e6xxx_port_write(...);

> +	if (err)
> +		return err;
> +
> +	return 0;
> +}

Re: [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Ido Schimmel]

On Fri, Feb 18, 2022 at 04:51:48PM +0100, Hans Schultz wrote:
> These tests check that the basic locked port feature works, so that no 'host'
> can communicate (ping) through a locked port unless the MAC address of the
> 'host' interface is in the forwarding database of the bridge.

Thanks for adding the test. I assume this was tested with both mv88e6xxx
and veth?

> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  .../testing/selftests/net/forwarding/Makefile |   1 +
>  .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>  3 files changed, 191 insertions(+)
>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index 72ee644d47bf..8fa97ae9af9e 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>  
>  TEST_PROGS = bridge_igmp.sh \
> +	bridge_locked_port.sh \
>  	bridge_port_isolation.sh \
>  	bridge_sticky_fdb.sh \
>  	bridge_vlan_aware.sh \
> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> new file mode 100755
> index 000000000000..d2805441b325
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> @@ -0,0 +1,174 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
> +NUM_NETIFS=4
> +CHECK_TC="no"
> +source lib.sh
> +
> +h1_create()
> +{
> +	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
> +	vrf_create "vrf-vlan-h1"
> +        ip link set dev vrf-vlan-h1 up
> +        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64

In the tests we try to use only addresses specified in RFC 5737. Instead
of 192.0.3.0/24 I suggest 198.51.100.0/24

> +}
> +
> +h1_destroy()
> +{
> +	vlan_destroy $h1 100
> +	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
> +}
> +
> +h2_create()
> +{
> +	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
> +	vrf_create "vrf-vlan-h2"
> +	ip link set dev vrf-vlan-h2 up
> +	vlan_create $h2 100 vrf-vlan-h2 192.0.3.2/24 2001:db8:3::2/64
> +}
> +
> +h2_destroy()
> +{
> +	vlan_destroy $h2 100
> +	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
> +}
> +
> +switch_create()
> +{
> +	ip link add dev br0 type bridge vlan_filtering 1
> +
> +	ip link set dev $swp1 master br0
> +	ip link set dev $swp2 master br0
> +
> +	ip link set dev br0 up
> +	ip link set dev $swp1 up
> +	ip link set dev $swp2 up
> +
> +	bridge link set dev $swp1 learning off
> +}
> +
> +switch_destroy()
> +{
> +	ip link set dev $swp2 down
> +	ip link set dev $swp1 down
> +
> +	ip link del dev br0
> +}
> +
> +setup_prepare()
> +{
> +	h1=${NETIFS[p1]}
> +	swp1=${NETIFS[p2]}
> +
> +	swp2=${NETIFS[p3]}
> +	h2=${NETIFS[p4]}
> +
> +	vrf_prepare
> +
> +	h1_create
> +	h2_create
> +
> +	switch_create
> +}
> +
> +cleanup()
> +{
> +	pre_cleanup
> +
> +	switch_destroy
> +
> +	h2_destroy
> +	h1_destroy
> +
> +	vrf_cleanup
> +}
> +
> +ifaddr()

We already have mac_get()

> +{
> +	ip -br link show dev "$1" | awk '{ print($3); }'
> +}
> +
> +locked_port_ipv4()
> +{
> +	RET=0
> +
> +	check_locked_port_support || return 0
> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "Ping didn't work when it should have"

Better to use unique error messages that pinpoint the problem:

"Ping did not work before locking port"

> +
> +	bridge link set dev $swp1 locked on
> +
> +	ping_do $h1 192.0.2.2
> +	check_fail $? "Ping worked when it should not have"

"Ping worked after locking port, but before adding a FDB entry"

> +
> +	bridge fdb add `ifaddr $h1` dev $swp1 master static

bridge fdb add $(mac_get $h1) dev $swp1 master static

> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "Ping didn't work when it should have"

"Ping did not work after locking port and adding a FDB entry"

> +
> +	bridge link set dev $swp1 locked off
> +	bridge fdb del `ifaddr $h1` dev $swp1 master static

I suggest to add another test case here to see that ping works after
unlocking the port and removing the FDB entry

Same comments on the other test cases

> +	log_test "Locked port ipv4"
> +}
> +
> +locked_port_vlan()
> +{
> +	RET=0
> +
> +	check_locked_port_support || return 0
> +	check_vlan_filtering_support || return 0

Why this check is needed? The bridge was already created with
"vlan_filtering 1"

> +
> +	bridge vlan add vid 100 dev $swp1 tagged

Not familiar with "tagged" keyword. I believe iproute2 ignores it.
Please drop it

> +	bridge vlan add vid 100 dev $swp2 tagged
> +
> +	ping_do $h1.100 192.0.3.2
> +	check_err $? "Ping didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked on
> +	ping_do $h1.100 192.0.3.2
> +	check_fail $? "Ping worked when it should not have"
> +
> +	bridge fdb add `ifaddr $h1` dev $swp1 vlan 100 master static
> +
> +	ping_do $h1.100 192.0.3.2
> +	check_err $? "Ping didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked off
> +	bridge vlan del vid 100 dev $swp1
> +	bridge vlan del vid 100 dev $swp2
> +	bridge fdb del `ifaddr $h1` dev $swp1 vlan 100 master static
> +	log_test "Locked port vlan"
> +}
> +
> +locked_port_ipv6()
> +{
> +	RET=0
> +	check_locked_port_support || return 0
> +
> +	ping6_do $h1 2001:db8:1::2
> +	check_err $? "Ping6 didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked on
> +
> +	ping6_do $h1 2001:db8:1::2
> +	check_fail $? "Ping worked when it should not have"
> +
> +	bridge fdb add `ifaddr $h1` dev $swp1 master static
> +	ping6_do $h1 2001:db8:1::2
> +	check_err $? "Ping didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked off
> +	bridge fdb del `ifaddr $h1` dev $swp1 master static
> +	log_test "Locked port ipv6"
> +}
> +
> +trap cleanup EXIT
> +
> +setup_prepare
> +setup_wait
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 7da783d6f453..9ded90f17ead 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -125,6 +125,22 @@ check_ethtool_lanes_support()
>  	fi
>  }
>  
> +check_locked_port_support()
> +{
> +        if ! bridge -d link show | grep -q " locked"; then
> +                echo "SKIP: iproute2 too old; Locked port feature not supported."
> +                return $ksft_skip
> +        fi
> +}
> +
> +check_vlan_filtering_support()
> +{
> +	if ! bridge -d vlan show | grep -q "state forwarding"; then
> +		echo "SKIP: vlan filtering not supported."
> +		return $ksft_skip
> +	fi
> +}
> +
>  if [[ "$(id -u)" -ne 0 ]]; then
>  	echo "SKIP: need root privileges"
>  	exit $ksft_skip
> -- 
> 2.30.2
> 

Re: [Bridge] [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Ido Schimmel]

On Fri, Feb 18, 2022 at 04:51:48PM +0100, Hans Schultz wrote:
> These tests check that the basic locked port feature works, so that no 'host'
> can communicate (ping) through a locked port unless the MAC address of the
> 'host' interface is in the forwarding database of the bridge.

Thanks for adding the test. I assume this was tested with both mv88e6xxx
and veth?

> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
> ---
>  .../testing/selftests/net/forwarding/Makefile |   1 +
>  .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>  tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>  3 files changed, 191 insertions(+)
>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> 
> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
> index 72ee644d47bf..8fa97ae9af9e 100644
> --- a/tools/testing/selftests/net/forwarding/Makefile
> +++ b/tools/testing/selftests/net/forwarding/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>  
>  TEST_PROGS = bridge_igmp.sh \
> +	bridge_locked_port.sh \
>  	bridge_port_isolation.sh \
>  	bridge_sticky_fdb.sh \
>  	bridge_vlan_aware.sh \
> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> new file mode 100755
> index 000000000000..d2805441b325
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> @@ -0,0 +1,174 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
> +NUM_NETIFS=4
> +CHECK_TC="no"
> +source lib.sh
> +
> +h1_create()
> +{
> +	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
> +	vrf_create "vrf-vlan-h1"
> +        ip link set dev vrf-vlan-h1 up
> +        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64

In the tests we try to use only addresses specified in RFC 5737. Instead
of 192.0.3.0/24 I suggest 198.51.100.0/24

> +}
> +
> +h1_destroy()
> +{
> +	vlan_destroy $h1 100
> +	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
> +}
> +
> +h2_create()
> +{
> +	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
> +	vrf_create "vrf-vlan-h2"
> +	ip link set dev vrf-vlan-h2 up
> +	vlan_create $h2 100 vrf-vlan-h2 192.0.3.2/24 2001:db8:3::2/64
> +}
> +
> +h2_destroy()
> +{
> +	vlan_destroy $h2 100
> +	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
> +}
> +
> +switch_create()
> +{
> +	ip link add dev br0 type bridge vlan_filtering 1
> +
> +	ip link set dev $swp1 master br0
> +	ip link set dev $swp2 master br0
> +
> +	ip link set dev br0 up
> +	ip link set dev $swp1 up
> +	ip link set dev $swp2 up
> +
> +	bridge link set dev $swp1 learning off
> +}
> +
> +switch_destroy()
> +{
> +	ip link set dev $swp2 down
> +	ip link set dev $swp1 down
> +
> +	ip link del dev br0
> +}
> +
> +setup_prepare()
> +{
> +	h1=${NETIFS[p1]}
> +	swp1=${NETIFS[p2]}
> +
> +	swp2=${NETIFS[p3]}
> +	h2=${NETIFS[p4]}
> +
> +	vrf_prepare
> +
> +	h1_create
> +	h2_create
> +
> +	switch_create
> +}
> +
> +cleanup()
> +{
> +	pre_cleanup
> +
> +	switch_destroy
> +
> +	h2_destroy
> +	h1_destroy
> +
> +	vrf_cleanup
> +}
> +
> +ifaddr()

We already have mac_get()

> +{
> +	ip -br link show dev "$1" | awk '{ print($3); }'
> +}
> +
> +locked_port_ipv4()
> +{
> +	RET=0
> +
> +	check_locked_port_support || return 0
> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "Ping didn't work when it should have"

Better to use unique error messages that pinpoint the problem:

"Ping did not work before locking port"

> +
> +	bridge link set dev $swp1 locked on
> +
> +	ping_do $h1 192.0.2.2
> +	check_fail $? "Ping worked when it should not have"

"Ping worked after locking port, but before adding a FDB entry"

> +
> +	bridge fdb add `ifaddr $h1` dev $swp1 master static

bridge fdb add $(mac_get $h1) dev $swp1 master static

> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "Ping didn't work when it should have"

"Ping did not work after locking port and adding a FDB entry"

> +
> +	bridge link set dev $swp1 locked off
> +	bridge fdb del `ifaddr $h1` dev $swp1 master static

I suggest to add another test case here to see that ping works after
unlocking the port and removing the FDB entry

Same comments on the other test cases

> +	log_test "Locked port ipv4"
> +}
> +
> +locked_port_vlan()
> +{
> +	RET=0
> +
> +	check_locked_port_support || return 0
> +	check_vlan_filtering_support || return 0

Why this check is needed? The bridge was already created with
"vlan_filtering 1"

> +
> +	bridge vlan add vid 100 dev $swp1 tagged

Not familiar with "tagged" keyword. I believe iproute2 ignores it.
Please drop it

> +	bridge vlan add vid 100 dev $swp2 tagged
> +
> +	ping_do $h1.100 192.0.3.2
> +	check_err $? "Ping didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked on
> +	ping_do $h1.100 192.0.3.2
> +	check_fail $? "Ping worked when it should not have"
> +
> +	bridge fdb add `ifaddr $h1` dev $swp1 vlan 100 master static
> +
> +	ping_do $h1.100 192.0.3.2
> +	check_err $? "Ping didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked off
> +	bridge vlan del vid 100 dev $swp1
> +	bridge vlan del vid 100 dev $swp2
> +	bridge fdb del `ifaddr $h1` dev $swp1 vlan 100 master static
> +	log_test "Locked port vlan"
> +}
> +
> +locked_port_ipv6()
> +{
> +	RET=0
> +	check_locked_port_support || return 0
> +
> +	ping6_do $h1 2001:db8:1::2
> +	check_err $? "Ping6 didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked on
> +
> +	ping6_do $h1 2001:db8:1::2
> +	check_fail $? "Ping worked when it should not have"
> +
> +	bridge fdb add `ifaddr $h1` dev $swp1 master static
> +	ping6_do $h1 2001:db8:1::2
> +	check_err $? "Ping didn't work when it should have"
> +
> +	bridge link set dev $swp1 locked off
> +	bridge fdb del `ifaddr $h1` dev $swp1 master static
> +	log_test "Locked port ipv6"
> +}
> +
> +trap cleanup EXIT
> +
> +setup_prepare
> +setup_wait
> +
> +tests_run
> +
> +exit $EXIT_STATUS
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 7da783d6f453..9ded90f17ead 100644
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -125,6 +125,22 @@ check_ethtool_lanes_support()
>  	fi
>  }
>  
> +check_locked_port_support()
> +{
> +        if ! bridge -d link show | grep -q " locked"; then
> +                echo "SKIP: iproute2 too old; Locked port feature not supported."
> +                return $ksft_skip
> +        fi
> +}
> +
> +check_vlan_filtering_support()
> +{
> +	if ! bridge -d vlan show | grep -q "state forwarding"; then
> +		echo "SKIP: vlan filtering not supported."
> +		return $ksft_skip
> +	fi
> +}
> +
>  if [[ "$(id -u)" -ne 0 ]]; then
>  	echo "SKIP: need root privileges"
>  	exit $ksft_skip
> -- 
> 2.30.2
> 

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

[Ido Schimmel]

On Fri, Feb 18, 2022 at 04:51:44PM +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>

With Nik's comment fixed:

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

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

[Ido Schimmel]

On Fri, Feb 18, 2022 at 04:51:44PM +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>

With Nik's comment fixed:

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

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

[Ido Schimmel]

On Fri, Feb 18, 2022 at 04:51:45PM +0100, 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.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [Bridge] [PATCH net-next v3 2/5] net: bridge: Add support for offloading of locked port flag

[Ido Schimmel]

On Fri, Feb 18, 2022 at 04:51:45PM +0100, 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.
> 
> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Ido Schimmel]

On Sat, Feb 19, 2022 at 12:00:34PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 18, 2022 at 04:51:47PM +0100, Hans Schultz wrote:
> > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> > index ab41619a809b..46b7381899a0 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_SA_FILT_MASK;
> > +	if (locked)
> > +		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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);
> 
> 	return mv88e6xxx_port_write(...);

Not familiar with mv88e6xxx, but shouldn't there be a rollback of
previous operations? Specifically mv88e6xxx_port_write()

> 
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}

Re: [Bridge] [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Ido Schimmel]

On Sat, Feb 19, 2022 at 12:00:34PM +0200, Vladimir Oltean wrote:
> On Fri, Feb 18, 2022 at 04:51:47PM +0100, Hans Schultz wrote:
> > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> > index ab41619a809b..46b7381899a0 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_SA_FILT_MASK;
> > +	if (locked)
> > +		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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);
> 
> 	return mv88e6xxx_port_write(...);

Not familiar with mv88e6xxx, but shouldn't there be a rollback of
previous operations? Specifically mv88e6xxx_port_write()

> 
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}

Re: [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Hans Schultz]

On sön, feb 20, 2022 at 11:12, Ido Schimmel <idosch@idosch.org> wrote:
> On Fri, Feb 18, 2022 at 04:51:48PM +0100, Hans Schultz wrote:
>> These tests check that the basic locked port feature works, so that no 'host'
>> can communicate (ping) through a locked port unless the MAC address of the
>> 'host' interface is in the forwarding database of the bridge.
>
> Thanks for adding the test. I assume this was tested with both mv88e6xxx
> and veth?
>
Yes, both cases have been tested. :-)

>> 
>> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
>> ---
>>  .../testing/selftests/net/forwarding/Makefile |   1 +
>>  .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>>  tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>>  3 files changed, 191 insertions(+)
>>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> 
>> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
>> index 72ee644d47bf..8fa97ae9af9e 100644
>> --- a/tools/testing/selftests/net/forwarding/Makefile
>> +++ b/tools/testing/selftests/net/forwarding/Makefile
>> @@ -1,6 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>>  
>>  TEST_PROGS = bridge_igmp.sh \
>> +	bridge_locked_port.sh \
>>  	bridge_port_isolation.sh \
>>  	bridge_sticky_fdb.sh \
>>  	bridge_vlan_aware.sh \
>> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> new file mode 100755
>> index 000000000000..d2805441b325
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> @@ -0,0 +1,174 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
>> +NUM_NETIFS=4
>> +CHECK_TC="no"
>> +source lib.sh
>> +
>> +h1_create()
>> +{
>> +	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
>> +	vrf_create "vrf-vlan-h1"
>> +        ip link set dev vrf-vlan-h1 up
>> +        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64
>
> In the tests we try to use only addresses specified in RFC 5737. Instead
> of 192.0.3.0/24 I suggest 198.51.100.0/24
>
>> +}
>> +
>> +h1_destroy()
>> +{
>> +	vlan_destroy $h1 100
>> +	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
>> +}
>> +
>> +h2_create()
>> +{
>> +	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
>> +	vrf_create "vrf-vlan-h2"
>> +	ip link set dev vrf-vlan-h2 up
>> +	vlan_create $h2 100 vrf-vlan-h2 192.0.3.2/24 2001:db8:3::2/64
>> +}
>> +
>> +h2_destroy()
>> +{
>> +	vlan_destroy $h2 100
>> +	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
>> +}
>> +
>> +switch_create()
>> +{
>> +	ip link add dev br0 type bridge vlan_filtering 1
>> +
>> +	ip link set dev $swp1 master br0
>> +	ip link set dev $swp2 master br0
>> +
>> +	ip link set dev br0 up
>> +	ip link set dev $swp1 up
>> +	ip link set dev $swp2 up
>> +
>> +	bridge link set dev $swp1 learning off
>> +}
>> +
>> +switch_destroy()
>> +{
>> +	ip link set dev $swp2 down
>> +	ip link set dev $swp1 down
>> +
>> +	ip link del dev br0
>> +}
>> +
>> +setup_prepare()
>> +{
>> +	h1=${NETIFS[p1]}
>> +	swp1=${NETIFS[p2]}
>> +
>> +	swp2=${NETIFS[p3]}
>> +	h2=${NETIFS[p4]}
>> +
>> +	vrf_prepare
>> +
>> +	h1_create
>> +	h2_create
>> +
>> +	switch_create
>> +}
>> +
>> +cleanup()
>> +{
>> +	pre_cleanup
>> +
>> +	switch_destroy
>> +
>> +	h2_destroy
>> +	h1_destroy
>> +
>> +	vrf_cleanup
>> +}
>> +
>> +ifaddr()
>
> We already have mac_get()
>
>> +{
>> +	ip -br link show dev "$1" | awk '{ print($3); }'
>> +}
>> +
>> +locked_port_ipv4()
>> +{
>> +	RET=0
>> +
>> +	check_locked_port_support || return 0
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "Ping didn't work when it should have"
>
> Better to use unique error messages that pinpoint the problem:
>
> "Ping did not work before locking port"
>
>> +
>> +	bridge link set dev $swp1 locked on
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_fail $? "Ping worked when it should not have"
>
> "Ping worked after locking port, but before adding a FDB entry"
>
>> +
>> +	bridge fdb add `ifaddr $h1` dev $swp1 master static
>
> bridge fdb add $(mac_get $h1) dev $swp1 master static
>
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "Ping didn't work when it should have"
>
> "Ping did not work after locking port and adding a FDB entry"
>
>> +
>> +	bridge link set dev $swp1 locked off
>> +	bridge fdb del `ifaddr $h1` dev $swp1 master static
>
> I suggest to add another test case here to see that ping works after
> unlocking the port and removing the FDB entry
>
> Same comments on the other test cases
>
>> +	log_test "Locked port ipv4"
>> +}
>> +
>> +locked_port_vlan()
>> +{
>> +	RET=0
>> +
>> +	check_locked_port_support || return 0
>> +	check_vlan_filtering_support || return 0
>
> Why this check is needed? The bridge was already created with
> "vlan_filtering 1"
>
>> +
>> +	bridge vlan add vid 100 dev $swp1 tagged
>
> Not familiar with "tagged" keyword. I believe iproute2 ignores it.
> Please drop it
>
>> +	bridge vlan add vid 100 dev $swp2 tagged
>> +
>> +	ping_do $h1.100 192.0.3.2
>> +	check_err $? "Ping didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked on
>> +	ping_do $h1.100 192.0.3.2
>> +	check_fail $? "Ping worked when it should not have"
>> +
>> +	bridge fdb add `ifaddr $h1` dev $swp1 vlan 100 master static
>> +
>> +	ping_do $h1.100 192.0.3.2
>> +	check_err $? "Ping didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked off
>> +	bridge vlan del vid 100 dev $swp1
>> +	bridge vlan del vid 100 dev $swp2
>> +	bridge fdb del `ifaddr $h1` dev $swp1 vlan 100 master static
>> +	log_test "Locked port vlan"
>> +}
>> +
>> +locked_port_ipv6()
>> +{
>> +	RET=0
>> +	check_locked_port_support || return 0
>> +
>> +	ping6_do $h1 2001:db8:1::2
>> +	check_err $? "Ping6 didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked on
>> +
>> +	ping6_do $h1 2001:db8:1::2
>> +	check_fail $? "Ping worked when it should not have"
>> +
>> +	bridge fdb add `ifaddr $h1` dev $swp1 master static
>> +	ping6_do $h1 2001:db8:1::2
>> +	check_err $? "Ping didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked off
>> +	bridge fdb del `ifaddr $h1` dev $swp1 master static
>> +	log_test "Locked port ipv6"
>> +}
>> +
>> +trap cleanup EXIT
>> +
>> +setup_prepare
>> +setup_wait
>> +
>> +tests_run
>> +
>> +exit $EXIT_STATUS
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 7da783d6f453..9ded90f17ead 100644
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -125,6 +125,22 @@ check_ethtool_lanes_support()
>>  	fi
>>  }
>>  
>> +check_locked_port_support()
>> +{
>> +        if ! bridge -d link show | grep -q " locked"; then
>> +                echo "SKIP: iproute2 too old; Locked port feature not supported."
>> +                return $ksft_skip
>> +        fi
>> +}
>> +
>> +check_vlan_filtering_support()
>> +{
>> +	if ! bridge -d vlan show | grep -q "state forwarding"; then
>> +		echo "SKIP: vlan filtering not supported."
>> +		return $ksft_skip
>> +	fi
>> +}
>> +
>>  if [[ "$(id -u)" -ne 0 ]]; then
>>  	echo "SKIP: need root privileges"
>>  	exit $ksft_skip
>> -- 
>> 2.30.2
>> 

Re: [Bridge] [PATCH net-next v3 5/5] selftests: forwarding: tests of locked port feature

[Hans Schultz]

On sön, feb 20, 2022 at 11:12, Ido Schimmel <idosch@idosch.org> wrote:
> On Fri, Feb 18, 2022 at 04:51:48PM +0100, Hans Schultz wrote:
>> These tests check that the basic locked port feature works, so that no 'host'
>> can communicate (ping) through a locked port unless the MAC address of the
>> 'host' interface is in the forwarding database of the bridge.
>
> Thanks for adding the test. I assume this was tested with both mv88e6xxx
> and veth?
>
Yes, both cases have been tested. :-)

>> 
>> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com>
>> ---
>>  .../testing/selftests/net/forwarding/Makefile |   1 +
>>  .../net/forwarding/bridge_locked_port.sh      | 174 ++++++++++++++++++
>>  tools/testing/selftests/net/forwarding/lib.sh |  16 ++
>>  3 files changed, 191 insertions(+)
>>  create mode 100755 tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> 
>> diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
>> index 72ee644d47bf..8fa97ae9af9e 100644
>> --- a/tools/testing/selftests/net/forwarding/Makefile
>> +++ b/tools/testing/selftests/net/forwarding/Makefile
>> @@ -1,6 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0+ OR MIT
>>  
>>  TEST_PROGS = bridge_igmp.sh \
>> +	bridge_locked_port.sh \
>>  	bridge_port_isolation.sh \
>>  	bridge_sticky_fdb.sh \
>>  	bridge_vlan_aware.sh \
>> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> new file mode 100755
>> index 000000000000..d2805441b325
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> @@ -0,0 +1,174 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
>> +NUM_NETIFS=4
>> +CHECK_TC="no"
>> +source lib.sh
>> +
>> +h1_create()
>> +{
>> +	simple_if_init $h1 192.0.2.1/24 2001:db8:1::1/64
>> +	vrf_create "vrf-vlan-h1"
>> +        ip link set dev vrf-vlan-h1 up
>> +        vlan_create $h1 100 vrf-vlan-h1 192.0.3.1/24 2001:db8:3::1/64
>
> In the tests we try to use only addresses specified in RFC 5737. Instead
> of 192.0.3.0/24 I suggest 198.51.100.0/24
>
>> +}
>> +
>> +h1_destroy()
>> +{
>> +	vlan_destroy $h1 100
>> +	simple_if_fini $h1 192.0.2.1/24 2001:db8:1::1/64
>> +}
>> +
>> +h2_create()
>> +{
>> +	simple_if_init $h2 192.0.2.2/24 2001:db8:1::2/64
>> +	vrf_create "vrf-vlan-h2"
>> +	ip link set dev vrf-vlan-h2 up
>> +	vlan_create $h2 100 vrf-vlan-h2 192.0.3.2/24 2001:db8:3::2/64
>> +}
>> +
>> +h2_destroy()
>> +{
>> +	vlan_destroy $h2 100
>> +	simple_if_fini $h2 192.0.2.2/24 2001:db8:1::2/64
>> +}
>> +
>> +switch_create()
>> +{
>> +	ip link add dev br0 type bridge vlan_filtering 1
>> +
>> +	ip link set dev $swp1 master br0
>> +	ip link set dev $swp2 master br0
>> +
>> +	ip link set dev br0 up
>> +	ip link set dev $swp1 up
>> +	ip link set dev $swp2 up
>> +
>> +	bridge link set dev $swp1 learning off
>> +}
>> +
>> +switch_destroy()
>> +{
>> +	ip link set dev $swp2 down
>> +	ip link set dev $swp1 down
>> +
>> +	ip link del dev br0
>> +}
>> +
>> +setup_prepare()
>> +{
>> +	h1=${NETIFS[p1]}
>> +	swp1=${NETIFS[p2]}
>> +
>> +	swp2=${NETIFS[p3]}
>> +	h2=${NETIFS[p4]}
>> +
>> +	vrf_prepare
>> +
>> +	h1_create
>> +	h2_create
>> +
>> +	switch_create
>> +}
>> +
>> +cleanup()
>> +{
>> +	pre_cleanup
>> +
>> +	switch_destroy
>> +
>> +	h2_destroy
>> +	h1_destroy
>> +
>> +	vrf_cleanup
>> +}
>> +
>> +ifaddr()
>
> We already have mac_get()
>
>> +{
>> +	ip -br link show dev "$1" | awk '{ print($3); }'
>> +}
>> +
>> +locked_port_ipv4()
>> +{
>> +	RET=0
>> +
>> +	check_locked_port_support || return 0
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "Ping didn't work when it should have"
>
> Better to use unique error messages that pinpoint the problem:
>
> "Ping did not work before locking port"
>
>> +
>> +	bridge link set dev $swp1 locked on
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_fail $? "Ping worked when it should not have"
>
> "Ping worked after locking port, but before adding a FDB entry"
>
>> +
>> +	bridge fdb add `ifaddr $h1` dev $swp1 master static
>
> bridge fdb add $(mac_get $h1) dev $swp1 master static
>
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "Ping didn't work when it should have"
>
> "Ping did not work after locking port and adding a FDB entry"
>
>> +
>> +	bridge link set dev $swp1 locked off
>> +	bridge fdb del `ifaddr $h1` dev $swp1 master static
>
> I suggest to add another test case here to see that ping works after
> unlocking the port and removing the FDB entry
>
> Same comments on the other test cases
>
>> +	log_test "Locked port ipv4"
>> +}
>> +
>> +locked_port_vlan()
>> +{
>> +	RET=0
>> +
>> +	check_locked_port_support || return 0
>> +	check_vlan_filtering_support || return 0
>
> Why this check is needed? The bridge was already created with
> "vlan_filtering 1"
>
>> +
>> +	bridge vlan add vid 100 dev $swp1 tagged
>
> Not familiar with "tagged" keyword. I believe iproute2 ignores it.
> Please drop it
>
>> +	bridge vlan add vid 100 dev $swp2 tagged
>> +
>> +	ping_do $h1.100 192.0.3.2
>> +	check_err $? "Ping didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked on
>> +	ping_do $h1.100 192.0.3.2
>> +	check_fail $? "Ping worked when it should not have"
>> +
>> +	bridge fdb add `ifaddr $h1` dev $swp1 vlan 100 master static
>> +
>> +	ping_do $h1.100 192.0.3.2
>> +	check_err $? "Ping didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked off
>> +	bridge vlan del vid 100 dev $swp1
>> +	bridge vlan del vid 100 dev $swp2
>> +	bridge fdb del `ifaddr $h1` dev $swp1 vlan 100 master static
>> +	log_test "Locked port vlan"
>> +}
>> +
>> +locked_port_ipv6()
>> +{
>> +	RET=0
>> +	check_locked_port_support || return 0
>> +
>> +	ping6_do $h1 2001:db8:1::2
>> +	check_err $? "Ping6 didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked on
>> +
>> +	ping6_do $h1 2001:db8:1::2
>> +	check_fail $? "Ping worked when it should not have"
>> +
>> +	bridge fdb add `ifaddr $h1` dev $swp1 master static
>> +	ping6_do $h1 2001:db8:1::2
>> +	check_err $? "Ping didn't work when it should have"
>> +
>> +	bridge link set dev $swp1 locked off
>> +	bridge fdb del `ifaddr $h1` dev $swp1 master static
>> +	log_test "Locked port ipv6"
>> +}
>> +
>> +trap cleanup EXIT
>> +
>> +setup_prepare
>> +setup_wait
>> +
>> +tests_run
>> +
>> +exit $EXIT_STATUS
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 7da783d6f453..9ded90f17ead 100644
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -125,6 +125,22 @@ check_ethtool_lanes_support()
>>  	fi
>>  }
>>  
>> +check_locked_port_support()
>> +{
>> +        if ! bridge -d link show | grep -q " locked"; then
>> +                echo "SKIP: iproute2 too old; Locked port feature not supported."
>> +                return $ksft_skip
>> +        fi
>> +}
>> +
>> +check_vlan_filtering_support()
>> +{
>> +	if ! bridge -d vlan show | grep -q "state forwarding"; then
>> +		echo "SKIP: vlan filtering not supported."
>> +		return $ksft_skip
>> +	fi
>> +}
>> +
>>  if [[ "$(id -u)" -ne 0 ]]; then
>>  	echo "SKIP: need root privileges"
>>  	exit $ksft_skip
>> -- 
>> 2.30.2
>> 

Re: [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Hans Schultz]

On sön, feb 20, 2022 at 11:21, Ido Schimmel <idosch@idosch.org> wrote:
> On Sat, Feb 19, 2022 at 12:00:34PM +0200, Vladimir Oltean wrote:
>> On Fri, Feb 18, 2022 at 04:51:47PM +0100, Hans Schultz wrote:
>> > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> > index ab41619a809b..46b7381899a0 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_SA_FILT_MASK;
>> > +	if (locked)
>> > +		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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);
>> 
>> 	return mv88e6xxx_port_write(...);
>
> Not familiar with mv88e6xxx, but shouldn't there be a rollback of
> previous operations? Specifically mv88e6xxx_port_write()
>

If a register write function fails, I don't think that it would make
sense to try and resolve the situation by additional register write
calls (rollback).

>> 
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	return 0;
>> > +}

Re: [Bridge] [PATCH net-next v3 4/5] net: dsa: mv88e6xxx: Add support for bridge port locked mode

[Hans Schultz]

On sön, feb 20, 2022 at 11:21, Ido Schimmel <idosch@idosch.org> wrote:
> On Sat, Feb 19, 2022 at 12:00:34PM +0200, Vladimir Oltean wrote:
>> On Fri, Feb 18, 2022 at 04:51:47PM +0100, Hans Schultz wrote:
>> > diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> > index ab41619a809b..46b7381899a0 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_SA_FILT_MASK;
>> > +	if (locked)
>> > +		reg |= MV88E6XXX_PORT_CTL0_SA_FILT_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);
>> 
>> 	return mv88e6xxx_port_write(...);
>
> Not familiar with mv88e6xxx, but shouldn't there be a rollback of
> previous operations? Specifically mv88e6xxx_port_write()
>

If a register write function fails, I don't think that it would make
sense to try and resolve the situation by additional register write
calls (rollback).

>> 
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	return 0;
>> > +}