Hans Schultz
2022-Mar-10 15:00 UTC
[Bridge] [PATCH net-next 3/3] net: dsa: mv88e6xxx: mac-auth/MAB implementation
On tor, mar 10, 2022 at 16:28, Vladimir Oltean <olteanv at gmail.com> wrote:> On Thu, Mar 10, 2022 at 03:23:20PM +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 <schultz.hans+netdev at gmail.com> >> --- >> 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 | 67 +++++++++++++++++++ >> .../net/dsa/mv88e6xxx/mv88e6xxx_switchdev.h | 20 ++++++ >> drivers/net/dsa/mv88e6xxx/port.c | 11 +++ >> drivers/net/dsa/mv88e6xxx/port.h | 1 + >> 9 files changed, 138 insertions(+), 7 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); > > Do we want to suppress the ATU miss violation warnings if we're going to > notify the bridge, or is it better to keep them for some reason? > My logic is that they're part of normal operation, so suppressing makes > sense. > >> + 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..e0ca452b6f86 >> --- /dev/null >> +++ b/drivers/net/dsa/mv88e6xxx/mv88e6xxx_switchdev.c >> @@ -0,0 +1,67 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * mv88e6xxx_switchdev.c >> + * >> + * Authors: >> + * Hans J. Schultz <hans.schultz at westermo.com> >> + * >> + */ >> + >> +#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); > > Since this is threaded interrupt context, I suppose it could race with > dsa_port_bridge_leave(). So it is best to check whether "brport" is NULL > or not. >Would something like: if (dsa_is_unused_port(chip->ds, port)) return -ENODATA; be appropriate and sufficient for that?> Speaking of races with dsa_port_bridge_leave().. does SWITCHDEV_FDB_ADD_TO_BRIDGE > not require rtnl_lock? >Yes, I will add that...>> + err = call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE, brport, &info.info, extack); > > It is buggy to pass an uninitialized on-stack extack, just pass NULL if > there's no one to consume it. > > Alternatively, if the bridge produces a valid extack message for errors > in this case (I haven't checked), it may be more useful to manually > print the extack._msg to the kernel log - see dsa_switch_sync_vlan_filtering() > for an example.I will look into which case is the most appropriate...> > I am a bit uncomfortable having every driver implement this ad-hoc and > potentially have a gazillion subtle bugs like these, could we have a > common function exported by DSA that deals with SWITCHDEV_FDB_ADD_TO_BRIDGE? >I think that would be a good idea, only that a bunch of other drivers already have implementations that would need changes. In general, I think there should be a more centralized thought-through expandable API in switchdev for stuff like this. It should also include the rtnl_lock.>> + if (err) >> + return err; >> + entry->portvec = MV88E6XXX_G1_ATU_DATA_PORT_VECTOR_NO_EGRESS; >> + return mv88e6xxx_g1_atu_loadpurge(chip, fid, entry); >> +} >> 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 <hans.schultz at westermo.com> >> + * >> + */ >> + >> +#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..6b375b0caa2c 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) >> { >> 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 >>
Vladimir Oltean
2022-Mar-10 15:07 UTC
[Bridge] [PATCH net-next 3/3] net: dsa: mv88e6xxx: mac-auth/MAB implementation
On Thu, Mar 10, 2022 at 04:00:52PM +0100, Hans Schultz wrote:> >> + brport = dsa_port_to_bridge_port(dp); > > > > Since this is threaded interrupt context, I suppose it could race with > > dsa_port_bridge_leave(). So it is best to check whether "brport" is NULL > > or not. > > > Would something like: > if (dsa_is_unused_port(chip->ds, port)) > return -ENODATA; > > be appropriate and sufficient for that?static inline struct net_device *dsa_port_to_bridge_port(const struct dsa_port *dp) { if (!dp->bridge) return NULL; if (dp->lag) return dp->lag->dev; else if (dp->hsr_dev) return dp->hsr_dev; return dp->slave; } Notice the "dp->bridge" check. The assignments are in dsa_port_bridge_create() and in dsa_port_bridge_destroy(). These functions assume rtnl_mutex protection. The question was how do you serialize with that, and why do you assume that dsa_port_to_bridge_port() returns non-NULL. So no, dsa_is_unused_port() would do absolutely nothing to help.