This patch set extends the locked port feature for devices
that are behind a locked port, but do not have the ability to
authorize themselves as a supplicant using IEEE 802.1X.
Such devices can be printers, meters or anything related to
fixed installations. Instead of 802.1X authorization, devices
can get access based on their MAC addresses being whitelisted.
For an authorization daemon to detect that a device is trying
to get access through a locked port, the bridge will add the
MAC address of the device to the FDB with a locked flag to it.
Thus the authorization daemon can catch the FDB add event and
check if the MAC address is in the whitelist and if so replace
the FDB entry without the locked flag enabled, and thus open
the port for the device.
This feature is known as MAC-Auth or MAC Authentication Bypass
(MAB) in Cisco terminology, where the full MAB concept involves
additional Cisco infrastructure for authorization. There is no
real authentication process, as the MAC address of the device
is the only input the authorization daemon, in the general
case, has to base the decision if to unlock the port or not.
With this patch set, an implementation of the offloaded case is
supplied for the mv88e6xxx driver. When a packet ingresses on
a locked port, an ATU miss violation event will occur. When
handling such ATU miss violation interrupts, the MAC address of
the device is added to the FDB with a zero destination port
vector (DPV) and the MAC address is communicated through the
switchdev layer to the bridge, so that a FDB entry with the
locked flag enabled can be added.
Hans Schultz (4):
net: bridge: add fdb flag to extent locked port feature
net: switchdev: add support for offloading of fdb locked flag
net: dsa: mv88e6xxx: mac-auth/MAB implementation
selftests: forwarding: add test of MAC-Auth Bypass to locked port
tests
drivers/net/dsa/mv88e6xxx/Makefile | 1 +
drivers/net/dsa/mv88e6xxx/chip.c | 10 +--
drivers/net/dsa/mv88e6xxx/chip.h | 5 ++
drivers/net/dsa/mv88e6xxx/global1.h | 1 +
drivers/net/dsa/mv88e6xxx/global1_atu.c | 29 ++++++-
.../net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c | 75 +++++++++++++++++++
.../net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h | 20 +++++
drivers/net/dsa/mv88e6xxx/port.c | 17 ++++-
drivers/net/dsa/mv88e6xxx/port.h | 1 +
include/net/switchdev.h | 3 +-
include/uapi/linux/neighbour.h | 1 +
net/bridge/br.c | 3 +-
net/bridge/br_fdb.c | 13 +++-
net/bridge/br_input.c | 10 ++-
net/bridge/br_private.h | 5 +-
.../net/forwarding/bridge_locked_port.sh | 29 ++++++-
16 files changed, 206 insertions(+), 17 deletions(-)
create mode 100644 drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c
create mode 100644 drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h
--
2.30.2
Verify that the MAC-Auth mechanism works by adding a FDB entry with the
locked flag set. denying access until the FDB entry is replaced with a
FDB entry without the locked flag set.
Signed-off-by: Hans Schultz <[email protected]>
---
.../net/forwarding/bridge_locked_port.sh | 29 ++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index 6e98efa6d371..2f9519e814b6 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -1,7 +1,7 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
-ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
+ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan locked_port_mab"
NUM_NETIFS=4
CHECK_TC="no"
source lib.sh
@@ -170,6 +170,33 @@ locked_port_ipv6()
log_test "Locked port ipv6"
}
+locked_port_mab()
+{
+ RET=0
+ check_locked_port_support || return 0
+
+ ping_do $h1 192.0.2.2
+ check_err $? "MAB: Ping did not work before locking port"
+
+ bridge link set dev $swp1 locked on
+ bridge link set dev $swp1 learning on
+
+ ping_do $h1 192.0.2.2
+ check_fail $? "MAB: Ping worked on port just locked"
+
+ if ! bridge fdb show | grep `mac_get $h1` | grep -q "locked"; then
+ RET=1
+ retmsg="MAB: No locked fdb entry after ping on locked port"
+ fi
+
+ bridge fdb del `mac_get $h1` dev $swp1 master
+ bridge fdb add `mac_get $h1` dev $swp1 master static
+
+ ping_do $h1 192.0.2.2
+ check_err $? "MAB: Ping did not work with fdb entry without locked flag"
+
+ log_test "Locked port MAB"
+}
trap cleanup EXIT
setup_prepare
--
2.30.2
Add an intermediate state for clients behind a locked port to allow for
possible opening of the port for said clients. This feature corresponds
to the Mac-Auth and MAC Authentication Bypass (MAB) named features. The
latter defined by Cisco.
Only the kernel can set this FDB entry flag, while userspace can read
the flag and remove it by deleting the FDB entry.
Signed-off-by: Hans Schultz <[email protected]>
---
include/uapi/linux/neighbour.h | 1 +
net/bridge/br_fdb.c | 6 ++++++
net/bridge/br_input.c | 10 +++++++++-
net/bridge/br_private.h | 3 ++-
4 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index db05fb55055e..a2df3b9b2f68 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -51,6 +51,7 @@ enum {
#define NTF_ROUTER (1 << 7)
/* Extended flags under NDA_FLAGS_EXT: */
#define NTF_EXT_MANAGED (1 << 0)
+#define NTF_EXT_LOCKED (1 << 1)
/*
* Neighbor Cache Entry States.
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6ccda68bd473..57ec559a85a7 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -105,6 +105,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
struct nda_cacheinfo ci;
struct nlmsghdr *nlh;
struct ndmsg *ndm;
+ u8 ext_flags = 0;
nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
if (nlh == NULL)
@@ -125,11 +126,16 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
ndm->ndm_flags |= NTF_EXT_LEARNED;
if (test_bit(BR_FDB_STICKY, &fdb->flags))
ndm->ndm_flags |= NTF_STICKY;
+ if (test_bit(BR_FDB_ENTRY_LOCKED, &fdb->flags))
+ ext_flags |= NTF_EXT_LOCKED;
if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
goto nla_put_failure;
if (nla_put_u32(skb, NDA_MASTER, br->dev->ifindex))
goto nla_put_failure;
+ if (nla_put_u8(skb, NDA_FLAGS_EXT, ext_flags))
+ goto nla_put_failure;
+
ci.ndm_used = jiffies_to_clock_t(now - fdb->used);
ci.ndm_confirmed = 0;
ci.ndm_updated = jiffies_to_clock_t(now - fdb->updated);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index e0c13fcc50ed..5ef25a496806 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -94,8 +94,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
if (!fdb_src || READ_ONCE(fdb_src->dst) != p ||
- test_bit(BR_FDB_LOCAL, &fdb_src->flags))
+ test_bit(BR_FDB_LOCAL, &fdb_src->flags) ||
+ test_bit(BR_FDB_ENTRY_LOCKED, &fdb_src->flags)) {
+ if (!fdb_src) {
+ unsigned long flags = 0;
+
+ set_bit(BR_FDB_ENTRY_LOCKED, &flags);
+ br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, flags);
+ }
goto drop;
+ }
}
nbp_switchdev_frame_mark(p, skb);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 48bc61ebc211..f5a0b68c4857 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -248,7 +248,8 @@ enum {
BR_FDB_ADDED_BY_EXT_LEARN,
BR_FDB_OFFLOADED,
BR_FDB_NOTIFY,
- BR_FDB_NOTIFY_INACTIVE
+ BR_FDB_NOTIFY_INACTIVE,
+ BR_FDB_ENTRY_LOCKED,
};
struct net_bridge_fdb_key {
--
2.30.2
On 17/03/2022 15:44, Ido Schimmel wrote:
> On Thu, Mar 17, 2022 at 10:38:59AM +0100, Hans Schultz wrote:
>> Add an intermediate state for clients behind a locked port to allow for
>> possible opening of the port for said clients. This feature corresponds
>> to the Mac-Auth and MAC Authentication Bypass (MAB) named features. The
>> latter defined by Cisco.
>> Only the kernel can set this FDB entry flag, while userspace can read
>> the flag and remove it by deleting the FDB entry.
>
> Can you explain where this flag is rejected by the kernel?
>
> Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for
> new flags we should do a better job and reject unsupported
> configurations. WDYT?
>
Definitely, I agree.
> The neighbour code will correctly reject the new flag due to
> 'NTF_EXT_MASK'.
On Thu, Mar 17, 2022 at 03:50:26PM +0100, Hans Schultz wrote:
> On tor, mar 17, 2022 at 15:44, Ido Schimmel <[email protected]> wrote:
> > On Thu, Mar 17, 2022 at 10:38:59AM +0100, Hans Schultz wrote:
> >> Add an intermediate state for clients behind a locked port to allow for
> >> possible opening of the port for said clients. This feature corresponds
> >> to the Mac-Auth and MAC Authentication Bypass (MAB) named features. The
> >> latter defined by Cisco.
> >> Only the kernel can set this FDB entry flag, while userspace can read
> >> the flag and remove it by deleting the FDB entry.
> >
> > Can you explain where this flag is rejected by the kernel?
> >
> Is it an effort to set the flag from iproute2 on adding a fdb entry?
I'm not sure what you are asking, but even if iproute2 can't set the
flag it doesn't mean the kernel shouldn't reject it
>
> > Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for
> > new flags we should do a better job and reject unsupported
> > configurations. WDYT?
> >
> > The neighbour code will correctly reject the new flag due to
> > 'NTF_EXT_MASK'.
This implementation for the Marvell mv88e6xxx chip series, is
based on handling ATU miss violations occurring when packets
ingress on a port that is locked. The mac address triggering
the ATU miss violation is communicated through switchdev to
the bridge module, which adds a fdb entry with the fdb locked
flag set.
Note: The locked port must have learning enabled for the ATU
miss violation to occur.
Signed-off-by: Hans Schultz <[email protected]>
---
drivers/net/dsa/mv88e6xxx/Makefile | 1 +
drivers/net/dsa/mv88e6xxx/chip.c | 10 +--
drivers/net/dsa/mv88e6xxx/chip.h | 5 ++
drivers/net/dsa/mv88e6xxx/global1.h | 1 +
drivers/net/dsa/mv88e6xxx/global1_atu.c | 29 ++++++-
.../net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c | 75 +++++++++++++++++++
.../net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h | 20 +++++
drivers/net/dsa/mv88e6xxx/port.c | 17 ++++-
drivers/net/dsa/mv88e6xxx/port.h | 1 +
9 files changed, 150 insertions(+), 9 deletions(-)
create mode 100644 drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c
create mode 100644 drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h
diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index c8eca2b6f959..3ca57709730d 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -15,3 +15,4 @@ mv88e6xxx-objs += port_hidden.o
mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
mv88e6xxx-objs += serdes.o
mv88e6xxx-objs += smi.o
+mv88e6xxx-objs += mv88e6xxx_switchdev.o
\ No newline at end of file
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 84b90fc36c58..e1b6bd738085 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1714,11 +1714,11 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
return err;
}
-static int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
- int (*cb)(struct mv88e6xxx_chip *chip,
- const struct mv88e6xxx_vtu_entry *entry,
- void *priv),
- void *priv)
+int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+ int (*cb)(struct mv88e6xxx_chip *chip,
+ const struct mv88e6xxx_vtu_entry *entry,
+ void *priv),
+ void *priv)
{
struct mv88e6xxx_vtu_entry entry = {
.vid = mv88e6xxx_max_vid(chip),
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 30b92a265613..64e8fc470fdf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -763,6 +763,11 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
mutex_unlock(&chip->reg_lock);
}
+int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+ int (*cb)(struct mv88e6xxx_chip *chip,
+ const struct mv88e6xxx_vtu_entry *entry,
+ void *priv),
+ void *priv);
int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
#endif /* _MV88E6XXX_CHIP_H */
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 2c1607c858a1..729cc0610d9a 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -136,6 +136,7 @@
#define MV88E6XXX_G1_ATU_DATA_TRUNK 0x8000
#define MV88E6XXX_G1_ATU_DATA_TRUNK_ID_MASK 0x00f0
#define MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_MASK 0x3ff0
+#define MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS 0x0000
#define MV88E6XXX_G1_ATU_DATA_STATE_MASK 0x000f
#define MV88E6XXX_G1_ATU_DATA_STATE_UC_UNUSED 0x0000
#define MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_1_OLDEST 0x0001
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 40bd67a5c8e9..afa54fe8667e 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -12,6 +12,8 @@
#include "chip.h"
#include "global1.h"
+#include "port.h"
+#include "mv88e6xxx_switchdev.h"
/* Offset 0x01: ATU FID Register */
@@ -114,6 +116,18 @@ static int mv88e6xxx_g1_atu_op_wait(struct mv88e6xxx_chip *chip)
return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_ATU_OP, bit, 0);
}
+static int mv88e6xxx_g1_read_atu_violation(struct mv88e6xxx_chip *chip)
+{
+ int err;
+
+ err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_ATU_OP,
+ MV88E6XXX_G1_ATU_OP_BUSY | MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+ if (err)
+ return err;
+
+ return mv88e6xxx_g1_atu_op_wait(chip);
+}
+
static int mv88e6xxx_g1_atu_op(struct mv88e6xxx_chip *chip, u16 fid, u16 op)
{
u16 val;
@@ -356,11 +370,11 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
int spid;
int err;
u16 val;
+ u16 fid;
mv88e6xxx_reg_lock(chip);
- err = mv88e6xxx_g1_atu_op(chip, 0,
- MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+ err = mv88e6xxx_g1_read_atu_violation(chip);
if (err)
goto out;
@@ -368,6 +382,10 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
if (err)
goto out;
+ err = mv88e6xxx_g1_read(chip, MV88E6352_G1_ATU_FID, &fid);
+ if (err)
+ goto out;
+
err = mv88e6xxx_g1_atu_data_read(chip, &entry);
if (err)
goto out;
@@ -396,6 +414,13 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
"ATU miss violation for %pM portvec %x spid %d\n",
entry.mac, entry.portvec, spid);
chip->ports[spid].atu_miss_violation++;
+ if (mv88e6xxx_port_is_locked(chip, chip->ports[spid].port))
+ err = mv88e6xxx_switchdev_handle_atu_miss_violation(chip,
+ chip->ports[spid].port,
+ &entry,
+ fid);
+ if (err)
+ goto out;
}
if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) {
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c b/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c
new file mode 100644
index 000000000000..574ae7680720
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * mv88e6xxx_switchdev.c
+ *
+ * Authors:
+ * Hans J. Schultz <[email protected]>
+ *
+ */
+
+#include <net/switchdev.h>
+#include "chip.h"
+#include "global1.h"
+
+struct mv88e6xxx_fid_search_ctx {
+ u16 fid_search;
+ u16 vid_found;
+};
+
+static int mv88e6xxx_find_vid_on_matching_fid(struct mv88e6xxx_chip *chip,
+ const struct mv88e6xxx_vtu_entry *entry,
+ void *priv)
+{
+ struct mv88e6xxx_fid_search_ctx *ctx = priv;
+
+ if (ctx->fid_search == entry->fid) {
+ ctx->vid_found = entry->vid;
+ return 1;
+ }
+ return 0;
+}
+
+int mv88e6xxx_switchdev_handle_atu_miss_violation(struct mv88e6xxx_chip *chip,
+ int port,
+ struct mv88e6xxx_atu_entry *entry,
+ u16 fid)
+{
+ struct switchdev_notifier_fdb_info info = {
+ .addr = entry->mac,
+ .vid = 0,
+ .added_by_user = false,
+ .is_local = false,
+ .offloaded = true,
+ .locked = true,
+ };
+ struct mv88e6xxx_fid_search_ctx ctx;
+ struct netlink_ext_ack *extack;
+ struct net_device *brport;
+ struct dsa_port *dp;
+ int err;
+
+ ctx.fid_search = fid;
+ err = mv88e6xxx_vtu_walk(chip, mv88e6xxx_find_vid_on_matching_fid, &ctx);
+ if (err < 0)
+ return err;
+ if (err == 1)
+ info.vid = ctx.vid_found;
+ else
+ return -ENODATA;
+
+ dp = dsa_to_port(chip->ds, port);
+ brport = dsa_port_to_bridge_port(dp);
+ if (!brport)
+ return -ENODEV;
+
+ rtnl_lock();
+ err = call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE, brport, &info.info, extack);
+ if (err)
+ goto out;
+ entry->portvec = MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS;
+ err = mv88e6xxx_g1_atu_loadpurge(chip, fid, entry);
+
+out:
+ rtnl_unlock();
+ return err;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h b/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h
new file mode 100644
index 000000000000..127f3098f745
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * mv88e6xxx_switchdev.h
+ *
+ * Authors:
+ * Hans J. Schultz <[email protected]>
+ *
+ */
+
+#ifndef DRIVERS_NET_DSA_MV88E6XXX_MV88E6XXX_SWITCHDEV_H_
+#define DRIVERS_NET_DSA_MV88E6XXX_MV88E6XXX_SWITCHDEV_H_
+
+#include <net/switchdev.h>
+
+int mv88e6xxx_switchdev_handle_atu_miss_violation(struct mv88e6xxx_chip *chip,
+ int port,
+ struct mv88e6xxx_atu_entry *entry,
+ u16 fid);
+
+#endif /* DRIVERS_NET_DSA_MV88E6XXX_MV88E6XXX_SWITCHDEV_H_ */
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 795b3128768f..4656a6a3e93e 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1239,6 +1239,17 @@ int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
return err;
}
+bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port)
+{
+ u16 reg;
+
+ if (mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, ®))
+ return false;
+ if (!(reg & MV88E6XXX_PORT_CTL0_SA_FILT_DROP_ON_LOCK))
+ return false;
+ return true;
+}
+
int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
bool locked)
{
@@ -1261,9 +1272,11 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
if (err)
return err;
- reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+ reg &= ~(MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
+ MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG);
if (locked)
- reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+ reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
+ MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG;
return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
}
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index e0a705d82019..09ea8f1615bb 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -374,6 +374,7 @@ 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);
+bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip, int port);
int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
bool locked);
--
2.30.2
Used for Mac-auth/MAB feature in the offloaded case.
Signed-off-by: Hans Schultz <[email protected]>
---
include/net/switchdev.h | 3 ++-
net/bridge/br.c | 3 ++-
net/bridge/br_fdb.c | 7 +++++--
net/bridge/br_private.h | 2 +-
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 3e424d40fae3..d5d923411f5e 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -229,7 +229,8 @@ struct switchdev_notifier_fdb_info {
u16 vid;
u8 added_by_user:1,
is_local:1,
- offloaded:1;
+ offloaded:1,
+ locked:1;
};
struct switchdev_notifier_port_obj_info {
diff --git a/net/bridge/br.c b/net/bridge/br.c
index b1dea3febeea..adcdbecbc218 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -166,7 +166,8 @@ static int br_switchdev_event(struct notifier_block *unused,
case SWITCHDEV_FDB_ADD_TO_BRIDGE:
fdb_info = ptr;
err = br_fdb_external_learn_add(br, p, fdb_info->addr,
- fdb_info->vid, false);
+ fdb_info->vid, false,
+ fdb_info->locked);
if (err) {
err = notifier_from_errno(err);
break;
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 57ec559a85a7..57aa1955d34d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -987,7 +987,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
"FDB entry towards bridge must be permanent");
return -EINVAL;
}
- err = br_fdb_external_learn_add(br, p, addr, vid, true);
+ err = br_fdb_external_learn_add(br, p, addr, vid, true, false);
} else {
spin_lock_bh(&br->hash_lock);
err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
@@ -1216,7 +1216,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
const unsigned char *addr, u16 vid,
- bool swdev_notify)
+ bool swdev_notify, bool locked)
{
struct net_bridge_fdb_entry *fdb;
bool modified = false;
@@ -1236,6 +1236,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
if (!p)
flags |= BIT(BR_FDB_LOCAL);
+ if (locked)
+ flags |= BIT(BR_FDB_ENTRY_LOCKED);
+
fdb = fdb_create(br, p, addr, vid, flags);
if (!fdb) {
err = -ENOMEM;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f5a0b68c4857..3275e33b112f 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -790,7 +790,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
const unsigned char *addr, u16 vid,
- bool swdev_notify);
+ bool swdev_notify, bool locked);
int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
const unsigned char *addr, u16 vid,
bool swdev_notify);
--
2.30.2
On Thu, Mar 17, 2022 at 10:38:59AM +0100, Hans Schultz wrote:
> Add an intermediate state for clients behind a locked port to allow for
> possible opening of the port for said clients. This feature corresponds
> to the Mac-Auth and MAC Authentication Bypass (MAB) named features. The
> latter defined by Cisco.
> Only the kernel can set this FDB entry flag, while userspace can read
> the flag and remove it by deleting the FDB entry.
Can you explain where this flag is rejected by the kernel?
Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for
new flags we should do a better job and reject unsupported
configurations. WDYT?
The neighbour code will correctly reject the new flag due to
'NTF_EXT_MASK'.
On Thu, Mar 17, 2022 at 10:39:01AM +0100, Hans Schultz wrote:
> +int mv88e6xxx_switchdev_handle_atu_miss_violation(struct mv88e6xxx_chip *chip,
> + int port,
> + struct mv88e6xxx_atu_entry *entry,
> + u16 fid)
> +{
> + struct switchdev_notifier_fdb_info info = {
> + .addr = entry->mac,
> + .vid = 0,
> + .added_by_user = false,
> + .is_local = false,
> + .offloaded = true,
> + .locked = true,
> + };
> + struct mv88e6xxx_fid_search_ctx ctx;
> + struct netlink_ext_ack *extack;
> + struct net_device *brport;
> + struct dsa_port *dp;
> + int err;
> +
> + ctx.fid_search = fid;
> + err = mv88e6xxx_vtu_walk(chip, mv88e6xxx_find_vid_on_matching_fid, &ctx);
> + if (err < 0)
> + return err;
> + if (err == 1)
> + info.vid = ctx.vid_found;
> + else
> + return -ENODATA;
> +
> + dp = dsa_to_port(chip->ds, port);
> + brport = dsa_port_to_bridge_port(dp);
> + if (!brport)
> + return -ENODEV;
dsa_port_to_bridge_port() must be under rtnl_lock().
On a different CPU rather than the one servicing the interrupt, the
rtnl_lock is held exactly by the user space command that triggers the
deletion of the bridge port.
The interrupt thread runs, calls dsa_port_to_bridge_port(), and finds
a non-NULL brport, because the bridge is still doing something else in
del_nbp(), it hasn't yet reached the netdev_upper_dev_unlink() function
which will trigger dsa_port_bridge_leave() -> dsa_port_bridge_destroy().
So you continue bravely, and you call rtnl_lock() below. This will block
until the "ip" command finishes. When you acquire the rtnl_lock however,
the brport is no longer valid, because you have waited for the user
space command to finish.
Best case, the bridge port deletion command was "ip link set lan0 nomaster".
So "brport" is "lan0", you call SWITCHDEV_FDB_ADD_TO_BRIDGE, the bridge
doesn't recognize it as a bridge port, says "huh, weird" and carries on.
Worst case, "brport" was an offloaded LAG device which was a bridge
port, and when it got destroyed by "ip link del bond0", the bridge port
got destroyed too. So at this stage, you have a use-after-free because
bond0 no longer exists.
> +
> + rtnl_lock();
> + err = call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE, brport, &info.info, extack);
> + if (err)
> + goto out;
> + entry->portvec = MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS;
> + err = mv88e6xxx_g1_atu_loadpurge(chip, fid, entry);
> +
> +out:
> + rtnl_unlock();
> + return err;
> +}
On Thu, 17 Mar 2022 10:39:01 +0100 Hans Schultz wrote:
> This implementation for the Marvell mv88e6xxx chip series, is
> based on handling ATU miss violations occurring when packets
> ingress on a port that is locked. The mac address triggering
> the ATU miss violation is communicated through switchdev to
> the bridge module, which adds a fdb entry with the fdb locked
> flag set.
> Note: The locked port must have learning enabled for the ATU
> miss violation to occur.
>
> Signed-off-by: Hans Schultz <[email protected]>
drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c:32:5: warning: no previous prototype for ‘mv88e6xxx_switchdev_handle_atu_miss_violation’ [-Wmissing-prototypes]
32 | int mv88e6xxx_switchdev_handle_atu_miss_violation(struct mv88e6xxx_chip *chip,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
On tor, mar 17, 2022 at 15:44, Ido Schimmel <[email protected]> wrote:
> On Thu, Mar 17, 2022 at 10:38:59AM +0100, Hans Schultz wrote:
>> Add an intermediate state for clients behind a locked port to allow for
>> possible opening of the port for said clients. This feature corresponds
>> to the Mac-Auth and MAC Authentication Bypass (MAB) named features. The
>> latter defined by Cisco.
>> Only the kernel can set this FDB entry flag, while userspace can read
>> the flag and remove it by deleting the FDB entry.
>
> Can you explain where this flag is rejected by the kernel?
>
Is it an effort to set the flag from iproute2 on adding a fdb entry?
> Nik, it seems the bridge ignores 'NDA_FLAGS_EXT', but I think that for
> new flags we should do a better job and reject unsupported
> configurations. WDYT?
>
> The neighbour code will correctly reject the new flag due to
> 'NTF_EXT_MASK'.
On tor, mar 17, 2022 at 16:57, Ido Schimmel <[email protected]> wrote:
> On Thu, Mar 17, 2022 at 10:39:02AM +0100, Hans Schultz wrote:
>> Verify that the MAC-Auth mechanism works by adding a FDB entry with the
>> locked flag set. denying access until the FDB entry is replaced with a
>> FDB entry without the locked flag set.
>>
>> Signed-off-by: Hans Schultz <[email protected]>
>> ---
>> .../net/forwarding/bridge_locked_port.sh | 29 ++++++++++++++++++-
>> 1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> index 6e98efa6d371..2f9519e814b6 100755
>> --- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
>> @@ -1,7 +1,7 @@
>> #!/bin/bash
>> # SPDX-License-Identifier: GPL-2.0
>>
>> -ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
>> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan locked_port_mab"
>> NUM_NETIFS=4
>> CHECK_TC="no"
>> source lib.sh
>> @@ -170,6 +170,33 @@ locked_port_ipv6()
>> log_test "Locked port ipv6"
>> }
>>
>> +locked_port_mab()
>> +{
>> + RET=0
>> + check_locked_port_support || return 0
>> +
>> + ping_do $h1 192.0.2.2
>> + check_err $? "MAB: Ping did not work before locking port"
>> +
>> + bridge link set dev $swp1 locked on
>> + bridge link set dev $swp1 learning on
>> +
>> + ping_do $h1 192.0.2.2
>> + check_fail $? "MAB: Ping worked on port just locked"
>> +
>> + if ! bridge fdb show | grep `mac_get $h1` | grep -q "locked"; then
>> + RET=1
>> + retmsg="MAB: No locked fdb entry after ping on locked port"
>> + fi
>
> bridge fdb show | grep `mac_get $h1 | grep -q "locked"
> check_err $? "MAB: No locked fdb entry after ping on locked port"
>
>> +
>> + bridge fdb del `mac_get $h1` dev $swp1 master
>> + bridge fdb add `mac_get $h1` dev $swp1 master static
>
> bridge fdb replace `mac_get $h1` dev $swp1 master static
>
Unfortunately for some reason 'replace' does not work in several of the
tests, while when replaced with 'del+add', they work.
>> +
>> + ping_do $h1 192.0.2.2
>> + check_err $? "MAB: Ping did not work with fdb entry without locked flag"
>> +
>> + log_test "Locked port MAB"
>
> Clean up after the test to revert to initial state:
>
> bridge fdb del `mac_get $h1` dev $swp1 master
> bridge link set dev $swp1 locked off
>
>
>> +}
>> trap cleanup EXIT
>>
>> setup_prepare
>> --
>> 2.30.2
>>
On Fri, Mar 18, 2022 at 04:45:24PM +0100, Hans Schultz wrote:
> On tor, mar 17, 2022 at 16:57, Ido Schimmel <[email protected]> wrote:
> > On Thu, Mar 17, 2022 at 10:39:02AM +0100, Hans Schultz wrote:
> >> Verify that the MAC-Auth mechanism works by adding a FDB entry with the
> >> locked flag set. denying access until the FDB entry is replaced with a
> >> FDB entry without the locked flag set.
> >>
> >> Signed-off-by: Hans Schultz <[email protected]>
> >> ---
> >> .../net/forwarding/bridge_locked_port.sh | 29 ++++++++++++++++++-
> >> 1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> >> index 6e98efa6d371..2f9519e814b6 100755
> >> --- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> >> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> >> @@ -1,7 +1,7 @@
> >> #!/bin/bash
> >> # SPDX-License-Identifier: GPL-2.0
> >>
> >> -ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
> >> +ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan locked_port_mab"
> >> NUM_NETIFS=4
> >> CHECK_TC="no"
> >> source lib.sh
> >> @@ -170,6 +170,33 @@ locked_port_ipv6()
> >> log_test "Locked port ipv6"
> >> }
> >>
> >> +locked_port_mab()
> >> +{
> >> + RET=0
> >> + check_locked_port_support || return 0
> >> +
> >> + ping_do $h1 192.0.2.2
> >> + check_err $? "MAB: Ping did not work before locking port"
> >> +
> >> + bridge link set dev $swp1 locked on
> >> + bridge link set dev $swp1 learning on
> >> +
> >> + ping_do $h1 192.0.2.2
> >> + check_fail $? "MAB: Ping worked on port just locked"
> >> +
> >> + if ! bridge fdb show | grep `mac_get $h1` | grep -q "locked"; then
> >> + RET=1
> >> + retmsg="MAB: No locked fdb entry after ping on locked port"
> >> + fi
> >
> > bridge fdb show | grep `mac_get $h1 | grep -q "locked"
> > check_err $? "MAB: No locked fdb entry after ping on locked port"
> >
> >> +
> >> + bridge fdb del `mac_get $h1` dev $swp1 master
> >> + bridge fdb add `mac_get $h1` dev $swp1 master static
> >
> > bridge fdb replace `mac_get $h1` dev $swp1 master static
> >
> Unfortunately for some reason 'replace' does not work in several of the
> tests, while when replaced with 'del+add', they work.
Is it because the 'locked' flag is not removed following the replace? At
least I don't see where it's handled in fdb_add_entry(). If so, please
fix it and use "bridge fdb replace" in the test.
>
> >> +
> >> + ping_do $h1 192.0.2.2
> >> + check_err $? "MAB: Ping did not work with fdb entry without locked flag"
> >> +
> >> + log_test "Locked port MAB"
> >
> > Clean up after the test to revert to initial state:
> >
> > bridge fdb del `mac_get $h1` dev $swp1 master
> > bridge link set dev $swp1 locked off
> >
> >
> >> +}
> >> trap cleanup EXIT
> >>
> >> setup_prepare
> >> --
> >> 2.30.2
> >>
On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
> >> Does someone have an idea why there at this point is no option to add a
> >> dynamic fdb entry?
> >>
> >> The fdb added entries here do not age out, while the ATU entries do
> >> (after 5 min), resulting in unsynced ATU vs fdb.
> >
> > I think the expectation is to use br_fdb_external_learn_del() if the
> > externally learned entry expires. The bridge should not age by itself
> > FDB entries learned externally.
> >
>
> It seems to me that something is missing then?
> My tests using trafgen that I gave a report on to Lunn generated massive
> amounts of fdb entries, but after a while the ATU was clean and the fdb
> was still full of random entries...
I'm no longer sure where you are, sorry..
I think we discussed that you need to enable ATU age interrupts in order
to keep the ATU in sync with the bridge FDB? Which means either to
delete the locked FDB entries from the bridge when they age out in the
ATU, or to keep refreshing locked ATU entries.
So it seems that you're doing neither of those 2 things if you end up
with bridge FDB entries which are no longer in the ATU.
On ons, mar 23, 2022 at 14:35, Vladimir Oltean <[email protected]> wrote:
> On Wed, Mar 23, 2022 at 01:29:52PM +0100, Hans Schultz wrote:
>> On tor, mar 17, 2022 at 10:39, Hans Schultz <[email protected]> wrote:
>> > Used for Mac-auth/MAB feature in the offloaded case.
>> >
>> > Signed-off-by: Hans Schultz <[email protected]>
>> > ---
>> > include/net/switchdev.h | 3 ++-
>> > net/bridge/br.c | 3 ++-
>> > net/bridge/br_fdb.c | 7 +++++--
>> > net/bridge/br_private.h | 2 +-
>> > 4 files changed, 10 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> > index 3e424d40fae3..d5d923411f5e 100644
>> > --- a/include/net/switchdev.h
>> > +++ b/include/net/switchdev.h
>> > @@ -229,7 +229,8 @@ struct switchdev_notifier_fdb_info {
>> > u16 vid;
>> > u8 added_by_user:1,
>> > is_local:1,
>> > - offloaded:1;
>> > + offloaded:1,
>> > + locked:1;
>> > };
>> >
>> > struct switchdev_notifier_port_obj_info {
>> > diff --git a/net/bridge/br.c b/net/bridge/br.c
>> > index b1dea3febeea..adcdbecbc218 100644
>> > --- a/net/bridge/br.c
>> > +++ b/net/bridge/br.c
>> > @@ -166,7 +166,8 @@ static int br_switchdev_event(struct notifier_block *unused,
>> > case SWITCHDEV_FDB_ADD_TO_BRIDGE:
>> > fdb_info = ptr;
>> > err = br_fdb_external_learn_add(br, p, fdb_info->addr,
>> > - fdb_info->vid, false);
>> > + fdb_info->vid, false,
>> > + fdb_info->locked);
>> > if (err) {
>> > err = notifier_from_errno(err);
>> > break;
>> > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> > index 57ec559a85a7..57aa1955d34d 100644
>> > --- a/net/bridge/br_fdb.c
>> > +++ b/net/bridge/br_fdb.c
>> > @@ -987,7 +987,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
>> > "FDB entry towards bridge must be permanent");
>> > return -EINVAL;
>> > }
>> > - err = br_fdb_external_learn_add(br, p, addr, vid, true);
>> > + err = br_fdb_external_learn_add(br, p, addr, vid, true,
>> > false);
>>
>> Does someone have an idea why there at this point is no option to add a
>> dynamic fdb entry?
>>
>> The fdb added entries here do not age out, while the ATU entries do
>> (after 5 min), resulting in unsynced ATU vs fdb.
>
> I think the expectation is to use br_fdb_external_learn_del() if the
> externally learned entry expires. The bridge should not age by itself
> FDB entries learned externally.
>
How is the mechanism supposed to work to remove fdb entries when ATU
entries age out?
>> > } else {
>> > spin_lock_bh(&br->hash_lock);
>> > err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
>> > @@ -1216,7 +1216,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
>> >
>> > int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>> > const unsigned char *addr, u16 vid,
>> > - bool swdev_notify)
>> > + bool swdev_notify, bool locked)
>> > {
>> > struct net_bridge_fdb_entry *fdb;
>> > bool modified = false;
>> > @@ -1236,6 +1236,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>> > if (!p)
>> > flags |= BIT(BR_FDB_LOCAL);
>> >
>> > + if (locked)
>> > + flags |= BIT(BR_FDB_ENTRY_LOCKED);
>> > +
>> > fdb = fdb_create(br, p, addr, vid, flags);
>> > if (!fdb) {
>> > err = -ENOMEM;
>> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> > index f5a0b68c4857..3275e33b112f 100644
>> > --- a/net/bridge/br_private.h
>> > +++ b/net/bridge/br_private.h
>> > @@ -790,7 +790,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
>> > void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
>> > int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>> > const unsigned char *addr, u16 vid,
>> > - bool swdev_notify);
>> > + bool swdev_notify, bool locked);
>> > int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
>> > const unsigned char *addr, u16 vid,
>> > bool swdev_notify);
>> > --
>> > 2.30.2
On ons, mar 23, 2022 at 14:35, Vladimir Oltean <[email protected]> wrote:
> On Wed, Mar 23, 2022 at 01:29:52PM +0100, Hans Schultz wrote:
>> On tor, mar 17, 2022 at 10:39, Hans Schultz <[email protected]> wrote:
>> > Used for Mac-auth/MAB feature in the offloaded case.
>> >
>> > Signed-off-by: Hans Schultz <[email protected]>
>> > ---
>> > include/net/switchdev.h | 3 ++-
>> > net/bridge/br.c | 3 ++-
>> > net/bridge/br_fdb.c | 7 +++++--
>> > net/bridge/br_private.h | 2 +-
>> > 4 files changed, 10 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> > index 3e424d40fae3..d5d923411f5e 100644
>> > --- a/include/net/switchdev.h
>> > +++ b/include/net/switchdev.h
>> > @@ -229,7 +229,8 @@ struct switchdev_notifier_fdb_info {
>> > u16 vid;
>> > u8 added_by_user:1,
>> > is_local:1,
>> > - offloaded:1;
>> > + offloaded:1,
>> > + locked:1;
>> > };
>> >
>> > struct switchdev_notifier_port_obj_info {
>> > diff --git a/net/bridge/br.c b/net/bridge/br.c
>> > index b1dea3febeea..adcdbecbc218 100644
>> > --- a/net/bridge/br.c
>> > +++ b/net/bridge/br.c
>> > @@ -166,7 +166,8 @@ static int br_switchdev_event(struct notifier_block *unused,
>> > case SWITCHDEV_FDB_ADD_TO_BRIDGE:
>> > fdb_info = ptr;
>> > err = br_fdb_external_learn_add(br, p, fdb_info->addr,
>> > - fdb_info->vid, false);
>> > + fdb_info->vid, false,
>> > + fdb_info->locked);
>> > if (err) {
>> > err = notifier_from_errno(err);
>> > break;
>> > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> > index 57ec559a85a7..57aa1955d34d 100644
>> > --- a/net/bridge/br_fdb.c
>> > +++ b/net/bridge/br_fdb.c
>> > @@ -987,7 +987,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
>> > "FDB entry towards bridge must be permanent");
>> > return -EINVAL;
>> > }
>> > - err = br_fdb_external_learn_add(br, p, addr, vid, true);
>> > + err = br_fdb_external_learn_add(br, p, addr, vid, true,
>> > false);
>>
>> Does someone have an idea why there at this point is no option to add a
>> dynamic fdb entry?
>>
>> The fdb added entries here do not age out, while the ATU entries do
>> (after 5 min), resulting in unsynced ATU vs fdb.
>
> I think the expectation is to use br_fdb_external_learn_del() if the
> externally learned entry expires. The bridge should not age by itself
> FDB entries learned externally.
>
It seems to me that something is missing then?
My tests using trafgen that I gave a report on to Lunn generated massive
amounts of fdb entries, but after a while the ATU was clean and the fdb
was still full of random entries...
>> > } else {
>> > spin_lock_bh(&br->hash_lock);
>> > err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
>> > @@ -1216,7 +1216,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
>> >
>> > int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>> > const unsigned char *addr, u16 vid,
>> > - bool swdev_notify)
>> > + bool swdev_notify, bool locked)
>> > {
>> > struct net_bridge_fdb_entry *fdb;
>> > bool modified = false;
>> > @@ -1236,6 +1236,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>> > if (!p)
>> > flags |= BIT(BR_FDB_LOCAL);
>> >
>> > + if (locked)
>> > + flags |= BIT(BR_FDB_ENTRY_LOCKED);
>> > +
>> > fdb = fdb_create(br, p, addr, vid, flags);
>> > if (!fdb) {
>> > err = -ENOMEM;
>> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> > index f5a0b68c4857..3275e33b112f 100644
>> > --- a/net/bridge/br_private.h
>> > +++ b/net/bridge/br_private.h
>> > @@ -790,7 +790,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
>> > void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
>> > int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
>> > const unsigned char *addr, u16 vid,
>> > - bool swdev_notify);
>> > + bool swdev_notify, bool locked);
>> > int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
>> > const unsigned char *addr, u16 vid,
>> > bool swdev_notify);
>> > --
>> > 2.30.2
On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
> On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
>> >> Does someone have an idea why there at this point is no option to add a
>> >> dynamic fdb entry?
>> >>
>> >> The fdb added entries here do not age out, while the ATU entries do
>> >> (after 5 min), resulting in unsynced ATU vs fdb.
>> >
>> > I think the expectation is to use br_fdb_external_learn_del() if the
>> > externally learned entry expires. The bridge should not age by itself
>> > FDB entries learned externally.
>> >
>>
>> It seems to me that something is missing then?
>> My tests using trafgen that I gave a report on to Lunn generated massive
>> amounts of fdb entries, but after a while the ATU was clean and the fdb
>> was still full of random entries...
>
> I'm no longer sure where you are, sorry..
> I think we discussed that you need to enable ATU age interrupts in order
> to keep the ATU in sync with the bridge FDB? Which means either to
> delete the locked FDB entries from the bridge when they age out in the
> ATU, or to keep refreshing locked ATU entries.
> So it seems that you're doing neither of those 2 things if you end up
> with bridge FDB entries which are no longer in the ATU.
Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
for it, so I assume it is something default?
On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
> >> >> Does someone have an idea why there at this point is no option to add a
> >> >> dynamic fdb entry?
> >> >>
> >> >> The fdb added entries here do not age out, while the ATU entries do
> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
> >> >
> >> > I think the expectation is to use br_fdb_external_learn_del() if the
> >> > externally learned entry expires. The bridge should not age by itself
> >> > FDB entries learned externally.
> >> >
> >>
> >> It seems to me that something is missing then?
> >> My tests using trafgen that I gave a report on to Lunn generated massive
> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
> >> was still full of random entries...
> >
> > I'm no longer sure where you are, sorry..
> > I think we discussed that you need to enable ATU age interrupts in order
> > to keep the ATU in sync with the bridge FDB? Which means either to
> > delete the locked FDB entries from the bridge when they age out in the
> > ATU, or to keep refreshing locked ATU entries.
> > So it seems that you're doing neither of those 2 things if you end up
> > with bridge FDB entries which are no longer in the ATU.
>
> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
> for it, so I assume it is something default?
No idea, but I can confirm that the out-of-reset value I see for
MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
rely on any reset defaults though.
On Fri, Mar 25, 2022 at 08:50:34AM +0100, Hans Schultz wrote:
> On tor, mar 24, 2022 at 16:27, Vladimir Oltean <[email protected]> wrote:
> > On Thu, Mar 24, 2022 at 12:23:39PM +0100, Hans Schultz wrote:
> >> On tor, mar 24, 2022 at 13:09, Vladimir Oltean <[email protected]> wrote:
> >> > On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
> >> >> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
> >> >> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
> >> >> >> >> Does someone have an idea why there at this point is no option to add a
> >> >> >> >> dynamic fdb entry?
> >> >> >> >>
> >> >> >> >> The fdb added entries here do not age out, while the ATU entries do
> >> >> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
> >> >> >> >
> >> >> >> > I think the expectation is to use br_fdb_external_learn_del() if the
> >> >> >> > externally learned entry expires. The bridge should not age by itself
> >> >> >> > FDB entries learned externally.
> >> >> >> >
> >> >> >>
> >> >> >> It seems to me that something is missing then?
> >> >> >> My tests using trafgen that I gave a report on to Lunn generated massive
> >> >> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
> >> >> >> was still full of random entries...
> >> >> >
> >> >> > I'm no longer sure where you are, sorry..
> >> >> > I think we discussed that you need to enable ATU age interrupts in order
> >> >> > to keep the ATU in sync with the bridge FDB? Which means either to
> >> >> > delete the locked FDB entries from the bridge when they age out in the
> >> >> > ATU, or to keep refreshing locked ATU entries.
> >> >> > So it seems that you're doing neither of those 2 things if you end up
> >> >> > with bridge FDB entries which are no longer in the ATU.
> >> >>
> >> >> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
> >> >> for it, so I assume it is something default?
> >> >
> >> > No idea, but I can confirm that the out-of-reset value I see for
> >> > MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
> >> > rely on any reset defaults though.
> >>
> >> I see no age out interrupts, even though the ports Age Out Int is on
> >> (PAV bit 14) on the locked port, and the ATU entries do age out (HoldAt1
> >> is off). Any idea why that can be?
> >>
> >> I combination with this I think it would be nice to have an ability to
> >> set the AgeOut time even though it is not per port but global.
> >
> > Sorry, I just don't know. Looking at the documentation for IntOnAgeOut,
> > I see it says that for an ATU entry to trigger an age out interrupt, the
> > port it's associated with must have IntOnAgeOut set.
> > But your locked ATU entries aren't associated with any port, they have
> > DPV=0, right? So will they never trigger any age out interrupt according
> > to this? I'm not clear.
>
> I think that's absolutely right. That leaves two options. Either "port
> 10" if it has IntOnAgeOut setting, or the reason why I wrote my comments
> in this part of the code, that it should be able to add a dynamic entry
> in the bridge module from the driver.
I'm sorry, I wasn't fully aware of the implications of the fact that
your 'locked' FDB entries have a DPV of all zeroes in hardware.
Practically, this means that while the locked bridge FDB entry is
associated with a bridge port, the ATU entry is associated with no port.
In turn, the hardware cannot ever true detect station migrations,
because it doesn't know which port this station migrates _from_ (you're
not telling it that). Every packet with this MAC SA is a station
migration, in effect, which you (for good reason) choose to ignore to
avoid denial of service.
Mark the locked (DPV=0) ATU entry as static, and you'll keep your CPU
clean of any ATU miss or member violation of this MAC SA. Read this as
"you'll need to call IT to ask them to remove it". Undesirable IMHO.
Mark the locked entry as non-static, and the entry will eventually
expire, with no interrupt to signal that - because any ATU age interrupt,
as mentioned, is fundamentally linked to a port.
You see this as a negative, and you're looking for ways to inform the
bridge driver that the locked FDB entry went away. But you aren't
looking at this the right way, I think. Making the mv88e6xxx driver
remove the locked FDB entry from the bridge seems like a non-goal now.
If you'd cache the locked ATU entry in the mv88e6xxx driver, and you'd
notify switchdev only if the entry is new to the cache, then you'd
actually still achieve something major. Yes, the bridge FDB will contain
locked FDB entries that aren't in the ATU. But that's because your
printer has been silent for X seconds. The policy for the printer still
hasn't changed, as far as the mv88e6xxx, or bridge, software drivers are
concerned. If the unauthorized printer says something again after the
locked ATU entry expires, the mv88e6xxx driver will find its MAC SA
in the cache of denied addresses, and reload the ATU. What this achieves
is that the number of ATU violation interrupts isn't proportional to the
number of packets sent by the printer, but with the ageing time you
configure for this ATU entry. You should be able to play with an
entry->state in the range of 1 -> 7 and get a good compromise between
responsiveness on station migrations and number of ATU interrupts to
service once the locked ATU entry is invalidated. In my opinion even the
quickest-to-expire entry->state of 1 is way better than letting every
packet spam the CPU. And you can always keep your cached locked ATU
entry in sync with the port that triggered the violation interrupt, and
figure out station migrations in software this way.
I hope I understood the hardware behavior correctly, I don't have any
direct experience with 802.1X as I mentioned, and only limited and
non-expert experience with Marvell hardware. This is just my
interpretation of some random documentation I found online.
On Thu, Mar 24, 2022 at 12:23:39PM +0100, Hans Schultz wrote:
> On tor, mar 24, 2022 at 13:09, Vladimir Oltean <[email protected]> wrote:
> > On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
> >> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
> >> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
> >> >> >> Does someone have an idea why there at this point is no option to add a
> >> >> >> dynamic fdb entry?
> >> >> >>
> >> >> >> The fdb added entries here do not age out, while the ATU entries do
> >> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
> >> >> >
> >> >> > I think the expectation is to use br_fdb_external_learn_del() if the
> >> >> > externally learned entry expires. The bridge should not age by itself
> >> >> > FDB entries learned externally.
> >> >> >
> >> >>
> >> >> It seems to me that something is missing then?
> >> >> My tests using trafgen that I gave a report on to Lunn generated massive
> >> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
> >> >> was still full of random entries...
> >> >
> >> > I'm no longer sure where you are, sorry..
> >> > I think we discussed that you need to enable ATU age interrupts in order
> >> > to keep the ATU in sync with the bridge FDB? Which means either to
> >> > delete the locked FDB entries from the bridge when they age out in the
> >> > ATU, or to keep refreshing locked ATU entries.
> >> > So it seems that you're doing neither of those 2 things if you end up
> >> > with bridge FDB entries which are no longer in the ATU.
> >>
> >> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
> >> for it, so I assume it is something default?
> >
> > No idea, but I can confirm that the out-of-reset value I see for
> > MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
> > rely on any reset defaults though.
>
> I see no age out interrupts, even though the ports Age Out Int is on
> (PAV bit 14) on the locked port, and the ATU entries do age out (HoldAt1
> is off). Any idea why that can be?
>
> I combination with this I think it would be nice to have an ability to
> set the AgeOut time even though it is not per port but global.
Sorry, I just don't know. Looking at the documentation for IntOnAgeOut,
I see it says that for an ATU entry to trigger an age out interrupt, the
port it's associated with must have IntOnAgeOut set.
But your locked ATU entries aren't associated with any port, they have
DPV=0, right? So will they never trigger any age out interrupt according
to this? I'm not clear.
On tor, mar 17, 2022 at 10:39, Hans Schultz <[email protected]> wrote:
> Used for Mac-auth/MAB feature in the offloaded case.
>
> Signed-off-by: Hans Schultz <[email protected]>
> ---
> include/net/switchdev.h | 3 ++-
> net/bridge/br.c | 3 ++-
> net/bridge/br_fdb.c | 7 +++++--
> net/bridge/br_private.h | 2 +-
> 4 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 3e424d40fae3..d5d923411f5e 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -229,7 +229,8 @@ struct switchdev_notifier_fdb_info {
> u16 vid;
> u8 added_by_user:1,
> is_local:1,
> - offloaded:1;
> + offloaded:1,
> + locked:1;
> };
>
> struct switchdev_notifier_port_obj_info {
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index b1dea3febeea..adcdbecbc218 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -166,7 +166,8 @@ static int br_switchdev_event(struct notifier_block *unused,
> case SWITCHDEV_FDB_ADD_TO_BRIDGE:
> fdb_info = ptr;
> err = br_fdb_external_learn_add(br, p, fdb_info->addr,
> - fdb_info->vid, false);
> + fdb_info->vid, false,
> + fdb_info->locked);
> if (err) {
> err = notifier_from_errno(err);
> break;
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 57ec559a85a7..57aa1955d34d 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -987,7 +987,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
> "FDB entry towards bridge must be permanent");
> return -EINVAL;
> }
> - err = br_fdb_external_learn_add(br, p, addr, vid, true);
> + err = br_fdb_external_learn_add(br, p, addr, vid, true,
> false);
Does someone have an idea why there at this point is no option to add a
dynamic fdb entry?
The fdb added entries here do not age out, while the ATU entries do
(after 5 min), resulting in unsynced ATU vs fdb.
> } else {
> spin_lock_bh(&br->hash_lock);
> err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> @@ -1216,7 +1216,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
>
> int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> const unsigned char *addr, u16 vid,
> - bool swdev_notify)
> + bool swdev_notify, bool locked)
> {
> struct net_bridge_fdb_entry *fdb;
> bool modified = false;
> @@ -1236,6 +1236,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> if (!p)
> flags |= BIT(BR_FDB_LOCAL);
>
> + if (locked)
> + flags |= BIT(BR_FDB_ENTRY_LOCKED);
> +
> fdb = fdb_create(br, p, addr, vid, flags);
> if (!fdb) {
> err = -ENOMEM;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index f5a0b68c4857..3275e33b112f 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -790,7 +790,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
> void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
> int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> const unsigned char *addr, u16 vid,
> - bool swdev_notify);
> + bool swdev_notify, bool locked);
> int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
> const unsigned char *addr, u16 vid,
> bool swdev_notify);
> --
> 2.30.2
On tor, mar 24, 2022 at 13:09, Vladimir Oltean <[email protected]> wrote:
> On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
>> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
>> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
>> >> >> Does someone have an idea why there at this point is no option to add a
>> >> >> dynamic fdb entry?
>> >> >>
>> >> >> The fdb added entries here do not age out, while the ATU entries do
>> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
>> >> >
>> >> > I think the expectation is to use br_fdb_external_learn_del() if the
>> >> > externally learned entry expires. The bridge should not age by itself
>> >> > FDB entries learned externally.
>> >> >
>> >>
>> >> It seems to me that something is missing then?
>> >> My tests using trafgen that I gave a report on to Lunn generated massive
>> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
>> >> was still full of random entries...
>> >
>> > I'm no longer sure where you are, sorry..
>> > I think we discussed that you need to enable ATU age interrupts in order
>> > to keep the ATU in sync with the bridge FDB? Which means either to
>> > delete the locked FDB entries from the bridge when they age out in the
>> > ATU, or to keep refreshing locked ATU entries.
>> > So it seems that you're doing neither of those 2 things if you end up
>> > with bridge FDB entries which are no longer in the ATU.
>>
>> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
>> for it, so I assume it is something default?
>
> No idea, but I can confirm that the out-of-reset value I see for
> MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
> rely on any reset defaults though.
I see no age out interrupts, even though the ports Age Out Int is on
(PAV bit 14) on the locked port, and the ATU entries do age out (HoldAt1
is off). Any idea why that can be?
I combination with this I think it would be nice to have an ability to
set the AgeOut time even though it is not per port but global.
On Fri, Mar 25, 2022 at 02:48:36PM +0100, Hans Schultz wrote:
> > If you'd cache the locked ATU entry in the mv88e6xxx driver, and you'd
> > notify switchdev only if the entry is new to the cache, then you'd
> > actually still achieve something major. Yes, the bridge FDB will contain
> > locked FDB entries that aren't in the ATU. But that's because your
> > printer has been silent for X seconds. The policy for the printer still
> > hasn't changed, as far as the mv88e6xxx, or bridge, software drivers are
> > concerned. If the unauthorized printer says something again after the
> > locked ATU entry expires, the mv88e6xxx driver will find its MAC SA
> > in the cache of denied addresses, and reload the ATU. What this
> > achieves
>
> The driver will in this case just trigger a new miss violation and add
> the entry again I think.
> The problem with all this is that a malicious attack that spams the
> switch with random mac addresses will be able to DOS the device as any
> handling of the fdb will be too resource demanding. That is why it is
> needed to remove those fdb entries after a time out, which dynamic
> entries would serve.
An attacker sweeping through the 2^47 source MAC address range is a
problem regardless of the implementations proposed so far, no?
If unlimited growth of the mv88e6xxx locked ATU entry cache is a
concern (which it is), we could limit its size, and when we purge a
cached entry in software is also when we could emit a
SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
On fre, mar 25, 2022 at 16:00, Vladimir Oltean <[email protected]> wrote:
> On Fri, Mar 25, 2022 at 02:48:36PM +0100, Hans Schultz wrote:
>> > If you'd cache the locked ATU entry in the mv88e6xxx driver, and you'd
>> > notify switchdev only if the entry is new to the cache, then you'd
>> > actually still achieve something major. Yes, the bridge FDB will contain
>> > locked FDB entries that aren't in the ATU. But that's because your
>> > printer has been silent for X seconds. The policy for the printer still
>> > hasn't changed, as far as the mv88e6xxx, or bridge, software drivers are
>> > concerned. If the unauthorized printer says something again after the
>> > locked ATU entry expires, the mv88e6xxx driver will find its MAC SA
>> > in the cache of denied addresses, and reload the ATU. What this
>> > achieves
>>
>> The driver will in this case just trigger a new miss violation and add
>> the entry again I think.
>> The problem with all this is that a malicious attack that spams the
>> switch with random mac addresses will be able to DOS the device as any
>> handling of the fdb will be too resource demanding. That is why it is
>> needed to remove those fdb entries after a time out, which dynamic
>> entries would serve.
>
> An attacker sweeping through the 2^47 source MAC address range is a
> problem regardless of the implementations proposed so far, no?
The idea is to have a count on the number of locked entries in both the
ATU and the FDB, so that a limit on entries can be enforced.
> If unlimited growth of the mv88e6xxx locked ATU entry cache is a
> concern (which it is), we could limit its size, and when we purge a
> cached entry in software is also when we could emit a
> SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
I think the best would be dynamic entries in both the ATU and the FDB
for locked entries. How the two are kept in sync is another question,
but if there is a switchcore, it will be the 'master', so I don't think
the bridge module will need to tell the switchcore to remove entries in
that case. Or?
On tor, mar 24, 2022 at 16:27, Vladimir Oltean <[email protected]> wrote:
> On Thu, Mar 24, 2022 at 12:23:39PM +0100, Hans Schultz wrote:
>> On tor, mar 24, 2022 at 13:09, Vladimir Oltean <[email protected]> wrote:
>> > On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
>> >> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
>> >> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
>> >> >> >> Does someone have an idea why there at this point is no option to add a
>> >> >> >> dynamic fdb entry?
>> >> >> >>
>> >> >> >> The fdb added entries here do not age out, while the ATU entries do
>> >> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
>> >> >> >
>> >> >> > I think the expectation is to use br_fdb_external_learn_del() if the
>> >> >> > externally learned entry expires. The bridge should not age by itself
>> >> >> > FDB entries learned externally.
>> >> >> >
>> >> >>
>> >> >> It seems to me that something is missing then?
>> >> >> My tests using trafgen that I gave a report on to Lunn generated massive
>> >> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
>> >> >> was still full of random entries...
>> >> >
>> >> > I'm no longer sure where you are, sorry..
>> >> > I think we discussed that you need to enable ATU age interrupts in order
>> >> > to keep the ATU in sync with the bridge FDB? Which means either to
>> >> > delete the locked FDB entries from the bridge when they age out in the
>> >> > ATU, or to keep refreshing locked ATU entries.
>> >> > So it seems that you're doing neither of those 2 things if you end up
>> >> > with bridge FDB entries which are no longer in the ATU.
>> >>
>> >> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
>> >> for it, so I assume it is something default?
>> >
>> > No idea, but I can confirm that the out-of-reset value I see for
>> > MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
>> > rely on any reset defaults though.
>>
>> I see no age out interrupts, even though the ports Age Out Int is on
>> (PAV bit 14) on the locked port, and the ATU entries do age out (HoldAt1
>> is off). Any idea why that can be?
>>
>> I combination with this I think it would be nice to have an ability to
>> set the AgeOut time even though it is not per port but global.
>
> Sorry, I just don't know. Looking at the documentation for IntOnAgeOut,
> I see it says that for an ATU entry to trigger an age out interrupt, the
> port it's associated with must have IntOnAgeOut set.
> But your locked ATU entries aren't associated with any port, they have
> DPV=0, right? So will they never trigger any age out interrupt according
> to this? I'm not clear.
If it could be possible to add a dynamic entry to the bridge module from
the driver, that would be a solution, and since in the ATU in this case
ages out entries learned, I don't see why __br_fdb_add() insists that an
external learned entry must be permanent?
On Wed, Mar 23, 2022 at 01:29:52PM +0100, Hans Schultz wrote:
> On tor, mar 17, 2022 at 10:39, Hans Schultz <[email protected]> wrote:
> > Used for Mac-auth/MAB feature in the offloaded case.
> >
> > Signed-off-by: Hans Schultz <[email protected]>
> > ---
> > include/net/switchdev.h | 3 ++-
> > net/bridge/br.c | 3 ++-
> > net/bridge/br_fdb.c | 7 +++++--
> > net/bridge/br_private.h | 2 +-
> > 4 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> > index 3e424d40fae3..d5d923411f5e 100644
> > --- a/include/net/switchdev.h
> > +++ b/include/net/switchdev.h
> > @@ -229,7 +229,8 @@ struct switchdev_notifier_fdb_info {
> > u16 vid;
> > u8 added_by_user:1,
> > is_local:1,
> > - offloaded:1;
> > + offloaded:1,
> > + locked:1;
> > };
> >
> > struct switchdev_notifier_port_obj_info {
> > diff --git a/net/bridge/br.c b/net/bridge/br.c
> > index b1dea3febeea..adcdbecbc218 100644
> > --- a/net/bridge/br.c
> > +++ b/net/bridge/br.c
> > @@ -166,7 +166,8 @@ static int br_switchdev_event(struct notifier_block *unused,
> > case SWITCHDEV_FDB_ADD_TO_BRIDGE:
> > fdb_info = ptr;
> > err = br_fdb_external_learn_add(br, p, fdb_info->addr,
> > - fdb_info->vid, false);
> > + fdb_info->vid, false,
> > + fdb_info->locked);
> > if (err) {
> > err = notifier_from_errno(err);
> > break;
> > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > index 57ec559a85a7..57aa1955d34d 100644
> > --- a/net/bridge/br_fdb.c
> > +++ b/net/bridge/br_fdb.c
> > @@ -987,7 +987,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
> > "FDB entry towards bridge must be permanent");
> > return -EINVAL;
> > }
> > - err = br_fdb_external_learn_add(br, p, addr, vid, true);
> > + err = br_fdb_external_learn_add(br, p, addr, vid, true,
> > false);
>
> Does someone have an idea why there at this point is no option to add a
> dynamic fdb entry?
>
> The fdb added entries here do not age out, while the ATU entries do
> (after 5 min), resulting in unsynced ATU vs fdb.
I think the expectation is to use br_fdb_external_learn_del() if the
externally learned entry expires. The bridge should not age by itself
FDB entries learned externally.
> > } else {
> > spin_lock_bh(&br->hash_lock);
> > err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
> > @@ -1216,7 +1216,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
> >
> > int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> > const unsigned char *addr, u16 vid,
> > - bool swdev_notify)
> > + bool swdev_notify, bool locked)
> > {
> > struct net_bridge_fdb_entry *fdb;
> > bool modified = false;
> > @@ -1236,6 +1236,9 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> > if (!p)
> > flags |= BIT(BR_FDB_LOCAL);
> >
> > + if (locked)
> > + flags |= BIT(BR_FDB_ENTRY_LOCKED);
> > +
> > fdb = fdb_create(br, p, addr, vid, flags);
> > if (!fdb) {
> > err = -ENOMEM;
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index f5a0b68c4857..3275e33b112f 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -790,7 +790,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p);
> > void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p);
> > int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
> > const unsigned char *addr, u16 vid,
> > - bool swdev_notify);
> > + bool swdev_notify, bool locked);
> > int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
> > const unsigned char *addr, u16 vid,
> > bool swdev_notify);
> > --
> > 2.30.2
On tor, mar 24, 2022 at 16:27, Vladimir Oltean <[email protected]> wrote:
> On Thu, Mar 24, 2022 at 12:23:39PM +0100, Hans Schultz wrote:
>> On tor, mar 24, 2022 at 13:09, Vladimir Oltean <[email protected]> wrote:
>> > On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
>> >> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
>> >> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
>> >> >> >> Does someone have an idea why there at this point is no option to add a
>> >> >> >> dynamic fdb entry?
>> >> >> >>
>> >> >> >> The fdb added entries here do not age out, while the ATU entries do
>> >> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
>> >> >> >
>> >> >> > I think the expectation is to use br_fdb_external_learn_del() if the
>> >> >> > externally learned entry expires. The bridge should not age by itself
>> >> >> > FDB entries learned externally.
>> >> >> >
>> >> >>
>> >> >> It seems to me that something is missing then?
>> >> >> My tests using trafgen that I gave a report on to Lunn generated massive
>> >> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
>> >> >> was still full of random entries...
>> >> >
>> >> > I'm no longer sure where you are, sorry..
>> >> > I think we discussed that you need to enable ATU age interrupts in order
>> >> > to keep the ATU in sync with the bridge FDB? Which means either to
>> >> > delete the locked FDB entries from the bridge when they age out in the
>> >> > ATU, or to keep refreshing locked ATU entries.
>> >> > So it seems that you're doing neither of those 2 things if you end up
>> >> > with bridge FDB entries which are no longer in the ATU.
>> >>
>> >> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
>> >> for it, so I assume it is something default?
>> >
>> > No idea, but I can confirm that the out-of-reset value I see for
>> > MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
>> > rely on any reset defaults though.
>>
>> I see no age out interrupts, even though the ports Age Out Int is on
>> (PAV bit 14) on the locked port, and the ATU entries do age out (HoldAt1
>> is off). Any idea why that can be?
>>
>> I combination with this I think it would be nice to have an ability to
>> set the AgeOut time even though it is not per port but global.
>
> Sorry, I just don't know. Looking at the documentation for IntOnAgeOut,
> I see it says that for an ATU entry to trigger an age out interrupt, the
> port it's associated with must have IntOnAgeOut set.
> But your locked ATU entries aren't associated with any port, they have
> DPV=0, right? So will they never trigger any age out interrupt according
> to this? I'm not clear.
I think that's absolutely right. That leaves two options. Either "port
10" if it has IntOnAgeOut setting, or the reason why I wrote my comments
in this part of the code, that it should be able to add a dynamic entry
in the bridge module from the driver.
On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
> On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
>> >> Does someone have an idea why there at this point is no option to add a
>> >> dynamic fdb entry?
>> >>
>> >> The fdb added entries here do not age out, while the ATU entries do
>> >> (after 5 min), resulting in unsynced ATU vs fdb.
>> >
>> > I think the expectation is to use br_fdb_external_learn_del() if the
>> > externally learned entry expires. The bridge should not age by itself
>> > FDB entries learned externally.
>> >
>>
>> It seems to me that something is missing then?
>> My tests using trafgen that I gave a report on to Lunn generated massive
>> amounts of fdb entries, but after a while the ATU was clean and the fdb
>> was still full of random entries...
>
> I'm no longer sure where you are, sorry..
> I think we discussed that you need to enable ATU age interrupts in order
> to keep the ATU in sync with the bridge FDB? Which means either to
> delete the locked FDB entries from the bridge when they age out in the
> ATU, or to keep refreshing locked ATU entries.
> So it seems that you're doing neither of those 2 things if you end up
> with bridge FDB entries which are no longer in the ATU.
Right, there was much that needed my attention, so after the other
issues are taken care of, I can focus on this. So I thought there was
some general machanism in place already, but I see that Ineed to enable
the IntOnAgeOut interrupt and handle ATU age out violations.
On fre, mar 25, 2022 at 15:21, Vladimir Oltean <[email protected]> wrote:
> On Fri, Mar 25, 2022 at 08:50:34AM +0100, Hans Schultz wrote:
>> On tor, mar 24, 2022 at 16:27, Vladimir Oltean <[email protected]> wrote:
>> > On Thu, Mar 24, 2022 at 12:23:39PM +0100, Hans Schultz wrote:
>> >> On tor, mar 24, 2022 at 13:09, Vladimir Oltean <[email protected]> wrote:
>> >> > On Thu, Mar 24, 2022 at 11:32:08AM +0100, Hans Schultz wrote:
>> >> >> On ons, mar 23, 2022 at 16:43, Vladimir Oltean <[email protected]> wrote:
>> >> >> > On Wed, Mar 23, 2022 at 01:49:32PM +0100, Hans Schultz wrote:
>> >> >> >> >> Does someone have an idea why there at this point is no option to add a
>> >> >> >> >> dynamic fdb entry?
>> >> >> >> >>
>> >> >> >> >> The fdb added entries here do not age out, while the ATU entries do
>> >> >> >> >> (after 5 min), resulting in unsynced ATU vs fdb.
>> >> >> >> >
>> >> >> >> > I think the expectation is to use br_fdb_external_learn_del() if the
>> >> >> >> > externally learned entry expires. The bridge should not age by itself
>> >> >> >> > FDB entries learned externally.
>> >> >> >> >
>> >> >> >>
>> >> >> >> It seems to me that something is missing then?
>> >> >> >> My tests using trafgen that I gave a report on to Lunn generated massive
>> >> >> >> amounts of fdb entries, but after a while the ATU was clean and the fdb
>> >> >> >> was still full of random entries...
>> >> >> >
>> >> >> > I'm no longer sure where you are, sorry..
>> >> >> > I think we discussed that you need to enable ATU age interrupts in order
>> >> >> > to keep the ATU in sync with the bridge FDB? Which means either to
>> >> >> > delete the locked FDB entries from the bridge when they age out in the
>> >> >> > ATU, or to keep refreshing locked ATU entries.
>> >> >> > So it seems that you're doing neither of those 2 things if you end up
>> >> >> > with bridge FDB entries which are no longer in the ATU.
>> >> >>
>> >> >> Any idea why G2 offset 5 ATUAgeIntEn (bit 10) is set? There is no define
>> >> >> for it, so I assume it is something default?
>> >> >
>> >> > No idea, but I can confirm that the out-of-reset value I see for
>> >> > MV88E6XXX_G2_SWITCH_MGMT on 6190 and 6390 is 0x400. It's best not to
>> >> > rely on any reset defaults though.
>> >>
>> >> I see no age out interrupts, even though the ports Age Out Int is on
>> >> (PAV bit 14) on the locked port, and the ATU entries do age out (HoldAt1
>> >> is off). Any idea why that can be?
>> >>
>> >> I combination with this I think it would be nice to have an ability to
>> >> set the AgeOut time even though it is not per port but global.
>> >
>> > Sorry, I just don't know. Looking at the documentation for IntOnAgeOut,
>> > I see it says that for an ATU entry to trigger an age out interrupt, the
>> > port it's associated with must have IntOnAgeOut set.
>> > But your locked ATU entries aren't associated with any port, they have
>> > DPV=0, right? So will they never trigger any age out interrupt according
>> > to this? I'm not clear.
>>
>> I think that's absolutely right. That leaves two options. Either "port
>> 10" if it has IntOnAgeOut setting, or the reason why I wrote my comments
>> in this part of the code, that it should be able to add a dynamic entry
>> in the bridge module from the driver.
>
> I'm sorry, I wasn't fully aware of the implications of the fact that
> your 'locked' FDB entries have a DPV of all zeroes in hardware.
> Practically, this means that while the locked bridge FDB entry is
> associated with a bridge port, the ATU entry is associated with no port.
>
> In turn, the hardware cannot ever true detect station migrations,
> because it doesn't know which port this station migrates _from_ (you're
> not telling it that). Every packet with this MAC SA is a station
> migration, in effect, which you (for good reason) choose to ignore to
> avoid denial of service.
>
> Mark the locked (DPV=0) ATU entry as static, and you'll keep your CPU
> clean of any ATU miss or member violation of this MAC SA. Read this as
> "you'll need to call IT to ask them to remove it". Undesirable IMHO.
>
> Mark the locked entry as non-static, and the entry will eventually
> expire, with no interrupt to signal that - because any ATU age interrupt,
> as mentioned, is fundamentally linked to a port.
>
> You see this as a negative, and you're looking for ways to inform the
> bridge driver that the locked FDB entry went away. But you aren't
> looking at this the right way, I think. Making the mv88e6xxx driver
> remove the locked FDB entry from the bridge seems like a non-goal now.
>
> If you'd cache the locked ATU entry in the mv88e6xxx driver, and you'd
> notify switchdev only if the entry is new to the cache, then you'd
> actually still achieve something major. Yes, the bridge FDB will contain
> locked FDB entries that aren't in the ATU. But that's because your
> printer has been silent for X seconds. The policy for the printer still
> hasn't changed, as far as the mv88e6xxx, or bridge, software drivers are
> concerned. If the unauthorized printer says something again after the
> locked ATU entry expires, the mv88e6xxx driver will find its MAC SA
> in the cache of denied addresses, and reload the ATU. What this
> achieves
The driver will in this case just trigger a new miss violation and add
the entry again I think.
The problem with all this is that a malicious attack that spams the
switch with random mac addresses will be able to DOS the device as any
handling of the fdb will be too resource demanding. That is why it is
needed to remove those fdb entries after a time out, which dynamic
entries would serve.
> is that the number of ATU violation interrupts isn't proportional to the
> number of packets sent by the printer, but with the ageing time you
> configure for this ATU entry. You should be able to play with an
> entry->state in the range of 1 -> 7 and get a good compromise between
> responsiveness on station migrations and number of ATU interrupts to
> service once the locked ATU entry is invalidated. In my opinion even the
> quickest-to-expire entry->state of 1 is way better than letting every
> packet spam the CPU. And you can always keep your cached locked ATU
> entry in sync with the port that triggered the violation interrupt, and
> figure out station migrations in software this way.
>
> I hope I understood the hardware behavior correctly, I don't have any
> direct experience with 802.1X as I mentioned, and only limited and
> non-expert experience with Marvell hardware. This is just my
> interpretation of some random documentation I found online.
On Fri, Mar 25, 2022 at 05:01:59PM +0100, Hans Schultz wrote:
> > An attacker sweeping through the 2^47 source MAC address range is a
> > problem regardless of the implementations proposed so far, no?
>
> The idea is to have a count on the number of locked entries in both the
> ATU and the FDB, so that a limit on entries can be enforced.
I can agree with that.
Note that as far as I understand regular 802.1X, these locked FDB
entries are just bloatware if you don't need MAC authentication bypass,
because the source port is already locked, so it drops all traffic from
an unknown MAC SA except for the link-local packets necessary to run
EAPOL, which are trapped to the CPU.
So maybe user space should opt into the MAC authentication bypass
process, really, since that requires secure CPU-assisted learning, and
regular 802.1X doesn't. It's a real additional burden that shouldn't be
ignored or enabled by default.
> > If unlimited growth of the mv88e6xxx locked ATU entry cache is a
> > concern (which it is), we could limit its size, and when we purge a
> > cached entry in software is also when we could emit a
> > SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
>
> I think the best would be dynamic entries in both the ATU and the FDB
> for locked entries.
Making locked (DPV=0) ATU entries be dynamic (age out) makes sense.
Since you set the IgnoreWrongData for source ports, you suppress ATU
interrupts for this MAC SA, which in turn means that a station which is
unauthorized on port A can never redeem itself when it migrates to port B,
for which it does have an authorization, since software never receives
any notice that it has moved to a new port.
But making the locked bridge FDB entry be dynamic, why does it matter?
I'm not seeing this through. To denote that it can migrate, or to denote
that it can age out? These locked FDB entries are 'extern_learn', so
they aren't aged out by the bridge anyway, they are aged out by whomever
added them => in our case the SWITCHDEV_FDB_DEL_TO_BRIDGE that I mentioned.
> How the two are kept in sync is another question, but if there is a
> switchcore, it will be the 'master', so I don't think the bridge
> module will need to tell the switchcore to remove entries in that
> case. Or?
The bridge will certainly not *need* to tell the switch to delete a
locked FDB entry, but it certainly *can* (and this is in fact part of
the authorization process, replace an ATU entry with DPV=0 with an ATU
entry with DPV=BIT(port)).
I feel as if I'm missing the essence of your reply.
On Mon, Mar 28, 2022 at 09:38:33AM +0200, Hans Schultz wrote:
> On fre, mar 25, 2022 at 22:30, Vladimir Oltean <[email protected]> wrote:
> > On Fri, Mar 25, 2022 at 05:01:59PM +0100, Hans Schultz wrote:
> >> > An attacker sweeping through the 2^47 source MAC address range is a
> >> > problem regardless of the implementations proposed so far, no?
> >>
> >> The idea is to have a count on the number of locked entries in both the
> >> ATU and the FDB, so that a limit on entries can be enforced.
> >
> > I can agree with that.
> >
> > Note that as far as I understand regular 802.1X, these locked FDB
> > entries are just bloatware if you don't need MAC authentication bypass,
> > because the source port is already locked, so it drops all traffic from
> > an unknown MAC SA except for the link-local packets necessary to run
> > EAPOL, which are trapped to the CPU.
>
> 802.1X and MAC Auth can be completely seperated by hostapd listning
> directly on the locked port interface before entering the bridge.
I don't understand this, sorry. What do you mean "before entering the
bridge"?
> > So maybe user space should opt into the MAC authentication bypass
> > process, really, since that requires secure CPU-assisted learning, and
> > regular 802.1X doesn't. It's a real additional burden that shouldn't be
> > ignored or enabled by default.
> >
> >> > If unlimited growth of the mv88e6xxx locked ATU entry cache is a
> >> > concern (which it is), we could limit its size, and when we purge a
> >> > cached entry in software is also when we could emit a
> >> > SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
> >>
> >> I think the best would be dynamic entries in both the ATU and the FDB
> >> for locked entries.
> >
> > Making locked (DPV=0) ATU entries be dynamic (age out) makes sense.
> > Since you set the IgnoreWrongData for source ports, you suppress ATU
> > interrupts for this MAC SA, which in turn means that a station which is
> > unauthorized on port A can never redeem itself when it migrates to port B,
> > for which it does have an authorization, since software never receives
> > any notice that it has moved to a new port.
> >
> > But making the locked bridge FDB entry be dynamic, why does it matter?
> > I'm not seeing this through. To denote that it can migrate, or to denote
> > that it can age out? These locked FDB entries are 'extern_learn', so
> > they aren't aged out by the bridge anyway, they are aged out by whomever
> > added them => in our case the SWITCHDEV_FDB_DEL_TO_BRIDGE that I mentioned.
> >
> I think the FDB and the ATU should be as much in sync as possible, and
> the FDB definitely should not keep stale entries that only get removed
> by link down. The SWITCHDEV_FDB_DEL_TO_BRIDGE route would requre an
> interrupt when a entry ages out in the ATU, but we know that that cannot
> happen with DPV=0. Thus the need to add dynamic entries with
> SWITCHDEV_FDB_ADD_TO_BRIDGE.
So what is your suggestion exactly? You want the driver to notify the
locked FDB entry via FDB_ADD_TO_BRIDGE with the dynamic flag, and then
rely on the bridge's software ageing timer to delete it? How does that
deletion propagate back to the driver then? I'm unclear on the ownership
model you propose.
> >> How the two are kept in sync is another question, but if there is a
> >> switchcore, it will be the 'master', so I don't think the bridge
> >> module will need to tell the switchcore to remove entries in that
> >> case. Or?
> >
> > The bridge will certainly not *need* to tell the switch to delete a
> > locked FDB entry, but it certainly *can* (and this is in fact part of
> > the authorization process, replace an ATU entry with DPV=0 with an ATU
> > entry with DPV=BIT(port)).
>
> Yes you are right, but I was implicitly only regarding internal
> mechanisms in the 'bridge + switchcore', and not userspace netlink
> commands.
> >
> > I feel as if I'm missing the essence of your reply.
On fre, mar 25, 2022 at 22:30, Vladimir Oltean <[email protected]> wrote:
> On Fri, Mar 25, 2022 at 05:01:59PM +0100, Hans Schultz wrote:
>> > An attacker sweeping through the 2^47 source MAC address range is a
>> > problem regardless of the implementations proposed so far, no?
>>
>> The idea is to have a count on the number of locked entries in both the
>> ATU and the FDB, so that a limit on entries can be enforced.
>
> I can agree with that.
>
> Note that as far as I understand regular 802.1X, these locked FDB
> entries are just bloatware if you don't need MAC authentication bypass,
> because the source port is already locked, so it drops all traffic from
> an unknown MAC SA except for the link-local packets necessary to run
> EAPOL, which are trapped to the CPU.
802.1X and MAC Auth can be completely seperated by hostapd listning
directly on the locked port interface before entering the bridge.
>
> So maybe user space should opt into the MAC authentication bypass
> process, really, since that requires secure CPU-assisted learning, and
> regular 802.1X doesn't. It's a real additional burden that shouldn't be
> ignored or enabled by default.
>
>> > If unlimited growth of the mv88e6xxx locked ATU entry cache is a
>> > concern (which it is), we could limit its size, and when we purge a
>> > cached entry in software is also when we could emit a
>> > SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
>>
>> I think the best would be dynamic entries in both the ATU and the FDB
>> for locked entries.
>
> Making locked (DPV=0) ATU entries be dynamic (age out) makes sense.
> Since you set the IgnoreWrongData for source ports, you suppress ATU
> interrupts for this MAC SA, which in turn means that a station which is
> unauthorized on port A can never redeem itself when it migrates to port B,
> for which it does have an authorization, since software never receives
> any notice that it has moved to a new port.
>
> But making the locked bridge FDB entry be dynamic, why does it matter?
> I'm not seeing this through. To denote that it can migrate, or to denote
> that it can age out? These locked FDB entries are 'extern_learn', so
> they aren't aged out by the bridge anyway, they are aged out by whomever
> added them => in our case the SWITCHDEV_FDB_DEL_TO_BRIDGE that I mentioned.
>
I think the FDB and the ATU should be as much in sync as possible, and
the FDB definitely should not keep stale entries that only get removed
by link down. The SWITCHDEV_FDB_DEL_TO_BRIDGE route would requre an
interrupt when a entry ages out in the ATU, but we know that that cannot
happen with DPV=0. Thus the need to add dynamic entries with
SWITCHDEV_FDB_ADD_TO_BRIDGE.
>> How the two are kept in sync is another question, but if there is a
>> switchcore, it will be the 'master', so I don't think the bridge
>> module will need to tell the switchcore to remove entries in that
>> case. Or?
>
> The bridge will certainly not *need* to tell the switch to delete a
> locked FDB entry, but it certainly *can* (and this is in fact part of
> the authorization process, replace an ATU entry with DPV=0 with an ATU
> entry with DPV=BIT(port)).
Yes you are right, but I was implicitly only regarding internal
mechanisms in the 'bridge + switchcore', and not userspace netlink
commands.
>
> I feel as if I'm missing the essence of your reply.
On Mon, Mar 28, 2022 at 11:31:43AM +0200, Hans Schultz wrote:
> On m?n, mar 28, 2022 at 11:48, Vladimir Oltean <[email protected]> wrote:
> > On Mon, Mar 28, 2022 at 09:38:33AM +0200, Hans Schultz wrote:
> >> On fre, mar 25, 2022 at 22:30, Vladimir Oltean <[email protected]> wrote:
> >> > On Fri, Mar 25, 2022 at 05:01:59PM +0100, Hans Schultz wrote:
> >> >> > An attacker sweeping through the 2^47 source MAC address range is a
> >> >> > problem regardless of the implementations proposed so far, no?
> >> >>
> >> >> The idea is to have a count on the number of locked entries in both the
> >> >> ATU and the FDB, so that a limit on entries can be enforced.
> >> >
> >> > I can agree with that.
> >> >
> >> > Note that as far as I understand regular 802.1X, these locked FDB
> >> > entries are just bloatware if you don't need MAC authentication bypass,
> >> > because the source port is already locked, so it drops all traffic from
> >> > an unknown MAC SA except for the link-local packets necessary to run
> >> > EAPOL, which are trapped to the CPU.
> >>
> >> 802.1X and MAC Auth can be completely seperated by hostapd listning
> >> directly on the locked port interface before entering the bridge.
> >
> > I don't understand this, sorry. What do you mean "before entering the
> > bridge"?
> >
> RAW socket on network slave device.
But as far as the port and its driver are concerned, there is a lot of
unnecessary functionality going on in the background if you don't need
MAC authentication bypass. All non-EAPOL packets could be unauthorized
without CPU intervention by simply not enabling CPU-assisted secure
learning in the first place. You might consider cutting off some of that
overhead by making user space opt into secure learning.
> >> > So maybe user space should opt into the MAC authentication bypass
> >> > process, really, since that requires secure CPU-assisted learning, and
> >> > regular 802.1X doesn't. It's a real additional burden that shouldn't be
> >> > ignored or enabled by default.
> >> >
> >> >> > If unlimited growth of the mv88e6xxx locked ATU entry cache is a
> >> >> > concern (which it is), we could limit its size, and when we purge a
> >> >> > cached entry in software is also when we could emit a
> >> >> > SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
> >> >>
> >> >> I think the best would be dynamic entries in both the ATU and the FDB
> >> >> for locked entries.
> >> >
> >> > Making locked (DPV=0) ATU entries be dynamic (age out) makes sense.
> >> > Since you set the IgnoreWrongData for source ports, you suppress ATU
> >> > interrupts for this MAC SA, which in turn means that a station which is
> >> > unauthorized on port A can never redeem itself when it migrates to port B,
> >> > for which it does have an authorization, since software never receives
> >> > any notice that it has moved to a new port.
> >> >
> >> > But making the locked bridge FDB entry be dynamic, why does it matter?
> >> > I'm not seeing this through. To denote that it can migrate, or to denote
> >> > that it can age out? These locked FDB entries are 'extern_learn', so
> >> > they aren't aged out by the bridge anyway, they are aged out by whomever
> >> > added them => in our case the SWITCHDEV_FDB_DEL_TO_BRIDGE that I mentioned.
> >> >
> >> I think the FDB and the ATU should be as much in sync as possible, and
> >> the FDB definitely should not keep stale entries that only get removed
> >> by link down. The SWITCHDEV_FDB_DEL_TO_BRIDGE route would requre an
> >> interrupt when a entry ages out in the ATU, but we know that that cannot
> >> happen with DPV=0. Thus the need to add dynamic entries with
> >> SWITCHDEV_FDB_ADD_TO_BRIDGE.
> >
> > So what is your suggestion exactly? You want the driver to notify the
> > locked FDB entry via FDB_ADD_TO_BRIDGE with the dynamic flag, and then
> > rely on the bridge's software ageing timer to delete it? How does that
> > deletion propagate back to the driver then? I'm unclear on the ownership
> > model you propose.
> >
>
> As the FDB and the ATU will age out the entry with the same timeout,
> they will stay relatively in sync compared to the situation where the
> switchcore driver will not be able to notify the bridge that a zero DPV
> entry has aged out as it has no port association.
So if the DPV=0 ATU entry doesn't get refreshed when a packet hits it
(even to get dropped), then I suppose the drift between software and
hardware ageing timers could be kept more or less under control.
But you still need to change switchdev and the bridge driver to support
this pattern, and you need to make a compelling case for it, because the
lack of a FDB_DEL_TO_BRIDGE notifier _is_ a concern in the general case.
And if you say "well, you know, the reason why I don't need to emit the
FDB_DEL_TO_BRIDGE is because I lied about the FDB entry's port association
in the first place (during FDB_ADD_TO_BRIDGE), it really is associated
with no port rather than with the port I said, just go with it", well,
that might not be the strongest argument for a new kind of externally
learned FDB entry. Anyway I'll defer to bridge and switchdev maintainers.
> >> >> How the two are kept in sync is another question, but if there is a
> >> >> switchcore, it will be the 'master', so I don't think the bridge
> >> >> module will need to tell the switchcore to remove entries in that
> >> >> case. Or?
> >> >
> >> > The bridge will certainly not *need* to tell the switch to delete a
> >> > locked FDB entry, but it certainly *can* (and this is in fact part of
> >> > the authorization process, replace an ATU entry with DPV=0 with an ATU
> >> > entry with DPV=BIT(port)).
> >>
> >> Yes you are right, but I was implicitly only regarding internal
> >> mechanisms in the 'bridge + switchcore', and not userspace netlink
> >> commands.
> >> >
> >> > I feel as if I'm missing the essence of your reply.
On mån, mar 28, 2022 at 11:48, Vladimir Oltean <[email protected]> wrote:
> On Mon, Mar 28, 2022 at 09:38:33AM +0200, Hans Schultz wrote:
>> On fre, mar 25, 2022 at 22:30, Vladimir Oltean <[email protected]> wrote:
>> > On Fri, Mar 25, 2022 at 05:01:59PM +0100, Hans Schultz wrote:
>> >> > An attacker sweeping through the 2^47 source MAC address range is a
>> >> > problem regardless of the implementations proposed so far, no?
>> >>
>> >> The idea is to have a count on the number of locked entries in both the
>> >> ATU and the FDB, so that a limit on entries can be enforced.
>> >
>> > I can agree with that.
>> >
>> > Note that as far as I understand regular 802.1X, these locked FDB
>> > entries are just bloatware if you don't need MAC authentication bypass,
>> > because the source port is already locked, so it drops all traffic from
>> > an unknown MAC SA except for the link-local packets necessary to run
>> > EAPOL, which are trapped to the CPU.
>>
>> 802.1X and MAC Auth can be completely seperated by hostapd listning
>> directly on the locked port interface before entering the bridge.
>
> I don't understand this, sorry. What do you mean "before entering the
> bridge"?
>
RAW socket on network slave device.
>> > So maybe user space should opt into the MAC authentication bypass
>> > process, really, since that requires secure CPU-assisted learning, and
>> > regular 802.1X doesn't. It's a real additional burden that shouldn't be
>> > ignored or enabled by default.
>> >
>> >> > If unlimited growth of the mv88e6xxx locked ATU entry cache is a
>> >> > concern (which it is), we could limit its size, and when we purge a
>> >> > cached entry in software is also when we could emit a
>> >> > SWITCHDEV_FDB_DEL_TO_BRIDGE for it, right?
>> >>
>> >> I think the best would be dynamic entries in both the ATU and the FDB
>> >> for locked entries.
>> >
>> > Making locked (DPV=0) ATU entries be dynamic (age out) makes sense.
>> > Since you set the IgnoreWrongData for source ports, you suppress ATU
>> > interrupts for this MAC SA, which in turn means that a station which is
>> > unauthorized on port A can never redeem itself when it migrates to port B,
>> > for which it does have an authorization, since software never receives
>> > any notice that it has moved to a new port.
>> >
>> > But making the locked bridge FDB entry be dynamic, why does it matter?
>> > I'm not seeing this through. To denote that it can migrate, or to denote
>> > that it can age out? These locked FDB entries are 'extern_learn', so
>> > they aren't aged out by the bridge anyway, they are aged out by whomever
>> > added them => in our case the SWITCHDEV_FDB_DEL_TO_BRIDGE that I mentioned.
>> >
>> I think the FDB and the ATU should be as much in sync as possible, and
>> the FDB definitely should not keep stale entries that only get removed
>> by link down. The SWITCHDEV_FDB_DEL_TO_BRIDGE route would requre an
>> interrupt when a entry ages out in the ATU, but we know that that cannot
>> happen with DPV=0. Thus the need to add dynamic entries with
>> SWITCHDEV_FDB_ADD_TO_BRIDGE.
>
> So what is your suggestion exactly? You want the driver to notify the
> locked FDB entry via FDB_ADD_TO_BRIDGE with the dynamic flag, and then
> rely on the bridge's software ageing timer to delete it? How does that
> deletion propagate back to the driver then? I'm unclear on the ownership
> model you propose.
>
As the FDB and the ATU will age out the entry with the same timeout,
they will stay relatively in sync compared to the situation where the
switchcore driver will not be able to notify the bridge that a zero DPV
entry has aged out as it has no port association.
>> >> How the two are kept in sync is another question, but if there is a
>> >> switchcore, it will be the 'master', so I don't think the bridge
>> >> module will need to tell the switchcore to remove entries in that
>> >> case. Or?
>> >
>> > The bridge will certainly not *need* to tell the switch to delete a
>> > locked FDB entry, but it certainly *can* (and this is in fact part of
>> > the authorization process, replace an ATU entry with DPV=0 with an ATU
>> > entry with DPV=BIT(port)).
>>
>> Yes you are right, but I was implicitly only regarding internal
>> mechanisms in the 'bridge + switchcore', and not userspace netlink
>> commands.
>> >
>> > I feel as if I'm missing the essence of your reply.