lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJN1Kkw=fnYpqL79cEPOYG9_zQVE+HDvYP1GoK4UkprmJ5qy7w@mail.gmail.com>
Date: Thu, 22 Aug 2024 16:19:38 +0200
From: Paweł Dembicki <paweldembicki@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>, 
	Florian Fainelli <f.fainelli@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Russell King <linux@...linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: vsc73xx: implement FDB operations

wt., 13 sie 2024 o 13:34 Vladimir Oltean <olteanv@...il.com> napisał(a):
>
> On Sun, Aug 11, 2024 at 09:56:49PM +0200, Pawel Dembicki wrote:
> > This commit introduces implementations of three functions:
> > .port_fdb_dump
> > .port_fdb_add
> > .port_fdb_del
> >
> > The FDB database organization is the same as in other old Vitesse chips:
> > It has 2048 rows and 4 columns (buckets). The row index is calculated by
> > the hash function 'vsc73xx_calc_hash' and the FDB entry must be placed
> > exactly into row[hash]. The chip selects the row number by itself.
>
> You mean "selects the bucket" maybe?
>
> >
> > Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> > ---
> >  drivers/net/dsa/vitesse-vsc73xx-core.c | 302 +++++++++++++++++++++++++
> >  drivers/net/dsa/vitesse-vsc73xx.h      |   2 +
> >  2 files changed, 304 insertions(+)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index a82b550a9e40..7da1641b8bab 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -46,6 +46,8 @@
> >  #define VSC73XX_BLOCK_MII_EXTERNAL   0x1 /* External MDIO subblock */
> >
> >  #define CPU_PORT     6 /* CPU port */
> > +#define VSC73XX_NUM_FDB_RECORDS      2048
>
> Terminology issue perhaps, but do you call a "record" as something that
> holds 1 FDB entry, or 4? There should be 2048 * 4 records, and 2048 "rows"?
>
> There's also vsc73xx_port_read_mac_table_entry(), which calls an FDB
> "entry" an array of 4 addresses. Do you have a consistent name for a
> switch data structure that holds a single address?
>
> > +#define VSC73XX_NUM_BUCKETS  4
> >
> >  /* MAC Block registers */
> >  #define VSC73XX_MAC_CFG              0x00
> > @@ -197,6 +199,21 @@
> >  #define VSC73XX_SRCMASKS_MIRROR                      BIT(26)
> >  #define VSC73XX_SRCMASKS_PORTS_MASK          GENMASK(7, 0)
> >
> > +#define VSC73XX_MACHDATA_VID                 GENMASK(27, 16)
> > +#define VSC73XX_MACHDATA_VID_SHIFT           16
> > +#define VSC73XX_MACHDATA_MAC0_SHIFT          8
> > +#define VSC73XX_MACHDATA_MAC1_SHIFT          0
> > +#define VSC73XX_MACLDATA_MAC2_SHIFT          24
> > +#define VSC73XX_MACLDATA_MAC3_SHIFT          16
> > +#define VSC73XX_MACLDATA_MAC4_SHIFT          8
> > +#define VSC73XX_MACLDATA_MAC5_SHIFT          0
> > +#define VSC73XX_MAC_BYTE_MASK                        GENMASK(7, 0)
> > +
> > +#define VSC73XX_MACTINDX_SHADOW                      BIT(13)
> > +#define VSC73XX_MACTINDX_BUCKET_MASK         GENMASK(12, 11)
> > +#define VSC73XX_MACTINDX_BUCKET_MASK_SHIFT   11
> > +#define VSC73XX_MACTINDX_INDEX_MASK          GENMASK(10, 0)
> > +
> >  #define VSC73XX_MACACCESS_CPU_COPY           BIT(14)
> >  #define VSC73XX_MACACCESS_FWD_KILL           BIT(13)
> >  #define VSC73XX_MACACCESS_IGNORE_VLAN                BIT(12)
> > @@ -204,6 +221,7 @@
> >  #define VSC73XX_MACACCESS_VALID                      BIT(10)
> >  #define VSC73XX_MACACCESS_LOCKED             BIT(9)
> >  #define VSC73XX_MACACCESS_DEST_IDX_MASK              GENMASK(8, 3)
> > +#define VSC73XX_MACACCESS_DEST_IDX_MASK_SHIFT        3
> >  #define VSC73XX_MACACCESS_CMD_MASK           GENMASK(2, 0)
> >  #define VSC73XX_MACACCESS_CMD_IDLE           0
> >  #define VSC73XX_MACACCESS_CMD_LEARN          1
> > @@ -329,6 +347,13 @@ struct vsc73xx_counter {
> >       const char *name;
> >  };
> >
> > +struct vsc73xx_fdb {
> > +     u16 vid;
> > +     u8 port;
> > +     u8 mac[6];
>
> u8 mac[ETH_ALEN]
>
> > +     bool valid;
> > +};
> > +
> >  /* Counters are named according to the MIB standards where applicable.
> >   * Some counters are custom, non-standard. The standard counters are
> >   * named in accordance with RFC2819, RFC2021 and IEEE Std 802.3-2002 Annex
> > @@ -1829,6 +1854,278 @@ static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> >               vsc73xx_refresh_fwd_map(ds, port, state);
> >  }
> >
> > +static u16 vsc73xx_calc_hash(const unsigned char *addr, u16 vid)
> > +{
> > +     /* VID 5-0, MAC 47-44 */
> > +     u16 hash = ((vid & GENMASK(5, 0)) << 4) | (addr[0] >> 4);
> > +
> > +     /* MAC 43-33 */
> > +     hash ^= ((addr[0] & GENMASK(3, 0)) << 7) | (addr[1] >> 1);
> > +     /* MAC 32-22 */
> > +     hash ^= ((addr[1] & BIT(0)) << 10) | (addr[2] << 2) | (addr[3] >> 6);
> > +     /* MAC 21-11 */
> > +     hash ^= ((addr[3] & GENMASK(5, 0)) << 5) | (addr[4] >> 3);
> > +     /* MAC 10-0 */
> > +     hash ^= ((addr[4] & GENMASK(2, 0)) << 8) | addr[5];
> > +
> > +     return hash;
> > +}
> > +
> > +static int
> > +vsc73xx_port_wait_for_mac_table_cmd(struct vsc73xx *vsc)
> > +{
> > +     int ret, err;
> > +     u32 val;
> > +
> > +     ret = read_poll_timeout(vsc73xx_read, err,
> > +                             err < 0 ||
> > +                             ((val & VSC73XX_MACACCESS_CMD_MASK) ==
> > +                              VSC73XX_MACACCESS_CMD_IDLE),
> > +                             VSC73XX_POLL_SLEEP_US, VSC73XX_POLL_TIMEOUT_US,
> > +                             false, vsc, VSC73XX_BLOCK_ANALYZER,
> > +                             0, VSC73XX_MACACCESS, &val);
> > +     if (ret)
> > +             return ret;
> > +     return err;
> > +}
> > +
> > +static int vsc73xx_port_read_mac_table_entry(struct vsc73xx *vsc, u16 index,
> > +                                          struct vsc73xx_fdb *fdb)
> > +{
> > +     int ret, i;
> > +     u32 val;
> > +
> > +     if (!fdb)
> > +             return -EINVAL;
> > +     if (index >= VSC73XX_NUM_FDB_RECORDS)
> > +             return -EINVAL;
> > +
> > +     for (i = 0; i < VSC73XX_NUM_BUCKETS; i++) {
> > +             vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACTINDX,
> > +                           (i ? 0 : VSC73XX_MACTINDX_SHADOW) |
> > +                           i << VSC73XX_MACTINDX_BUCKET_MASK_SHIFT |
> > +                           index);
>
> Could you check for error codes from vsc73xx_read() and vsc73xx_write()
> as well? This is applicable to the entire patch.
>
> > +             ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > +                                 VSC73XX_MACACCESS,
> > +                                 VSC73XX_MACACCESS_CMD_MASK,
> > +                                 VSC73XX_MACACCESS_CMD_READ_ENTRY);
> > +             ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACACCESS,
> > +                          &val);
> > +             fdb[i].valid = val & VSC73XX_MACACCESS_VALID;
> > +             if (!fdb[i].valid)
> > +                     continue;
> > +
> > +             fdb[i].port = (val & VSC73XX_MACACCESS_DEST_IDX_MASK) >>
> > +                           VSC73XX_MACACCESS_DEST_IDX_MASK_SHIFT;
> > +
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACHDATA,
> > +                          &val);
> > +             fdb[i].vid = (val & VSC73XX_MACHDATA_VID) >>
> > +                          VSC73XX_MACHDATA_VID_SHIFT;
> > +             fdb[i].mac[0] = (val >> VSC73XX_MACHDATA_MAC0_SHIFT) &
> > +                             VSC73XX_MAC_BYTE_MASK;
> > +             fdb[i].mac[1] = (val >> VSC73XX_MACHDATA_MAC1_SHIFT) &
> > +                             VSC73XX_MAC_BYTE_MASK;
> > +
> > +             vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACLDATA,
> > +                          &val);
> > +             fdb[i].mac[2] = (val >> VSC73XX_MACLDATA_MAC2_SHIFT) &
> > +                             VSC73XX_MAC_BYTE_MASK;
> > +             fdb[i].mac[3] = (val >> VSC73XX_MACLDATA_MAC3_SHIFT) &
> > +                             VSC73XX_MAC_BYTE_MASK;
> > +             fdb[i].mac[4] = (val >> VSC73XX_MACLDATA_MAC4_SHIFT) &
> > +                             VSC73XX_MAC_BYTE_MASK;
> > +             fdb[i].mac[5] = (val >> VSC73XX_MACLDATA_MAC5_SHIFT) &
> > +                             VSC73XX_MAC_BYTE_MASK;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static void
> > +vsc73xx_fdb_insert_mac(struct vsc73xx *vsc, const unsigned char *addr, u16 vid)
> > +{
> > +     u32 val;
> > +
> > +     val = (vid << VSC73XX_MACHDATA_VID_SHIFT) & VSC73XX_MACHDATA_VID;
> > +     val |= (addr[0] << VSC73XX_MACHDATA_MAC0_SHIFT);
> > +     val |= (addr[1] << VSC73XX_MACHDATA_MAC1_SHIFT);
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACHDATA, val);
> > +
> > +     val = (addr[2] << VSC73XX_MACLDATA_MAC2_SHIFT);
> > +     val |= (addr[3] << VSC73XX_MACLDATA_MAC3_SHIFT);
> > +     val |= (addr[4] << VSC73XX_MACLDATA_MAC4_SHIFT);
> > +     val |= (addr[5] << VSC73XX_MACLDATA_MAC5_SHIFT);
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACLDATA, val);
> > +}
> > +
> > +static int vsc73xx_fdb_del_entry(struct vsc73xx *vsc, int port,
> > +                              const unsigned char *addr, u16 vid)
> > +{
> > +     struct vsc73xx_fdb fdb[VSC73XX_NUM_BUCKETS];
> > +     u16 hash = vsc73xx_calc_hash(addr, vid);
> > +     int bucket, ret;
> > +
> > +     mutex_lock(&vsc->fdb_lock);
> > +
> > +     ret = vsc73xx_port_read_mac_table_entry(vsc, hash, fdb);
> > +     if (ret)
> > +             goto err;
> > +
> > +     for (bucket = 0; bucket < VSC73XX_NUM_BUCKETS; bucket++) {
> > +             if (fdb[bucket].valid && fdb[bucket].port == port &&
> > +                 !memcmp(addr, fdb[bucket].mac, ETH_ALEN))
>
> ether_addr_equal()
>
> > +                     break;
> > +     }
> > +
> > +     if (bucket == VSC73XX_NUM_BUCKETS) {
> > +             /* Can't find MAC in MAC table */
> > +             ret = -ENODATA;
> > +             goto err;
> > +     }
> > +
> > +     vsc73xx_fdb_insert_mac(vsc, addr, vid);
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACTINDX, hash);
> > +
> > +     ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> > +     if (ret)
> > +             goto err;
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACACCESS,
> > +                         VSC73XX_MACACCESS_CMD_MASK,
> > +                         VSC73XX_MACACCESS_CMD_FORGET);
> > +     ret =  vsc73xx_port_wait_for_mac_table_cmd(vsc);
> > +err:
> > +     mutex_unlock(&vsc->fdb_lock);
> > +     return ret;
> > +}
> > +
> > +static int vsc73xx_fdb_add_entry(struct vsc73xx *vsc, int port,
> > +                              const unsigned char *addr, u16 vid)
> > +{
> > +     struct vsc73xx_fdb fdb[VSC73XX_NUM_BUCKETS];
> > +     u16 hash = vsc73xx_calc_hash(addr, vid);
> > +     int bucket, ret;
> > +     u32 val;
> > +
> > +     mutex_lock(&vsc->fdb_lock);
> > +
> > +     vsc73xx_port_read_mac_table_entry(vsc, hash, fdb);
> > +
> > +     for (bucket = 0; bucket < VSC73XX_NUM_BUCKETS; bucket++) {
> > +             if (!fdb[bucket].valid)
> > +                     break;
> > +     }
> > +
> > +     if (bucket == VSC73XX_NUM_BUCKETS) {
> > +             /* Bucket is full */
> > +             ret = -EOVERFLOW;
> > +             goto err;
> > +     }
> > +
> > +     vsc73xx_fdb_insert_mac(vsc, addr, vid);
> > +
> > +     vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACTINDX, hash);
> > +     ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> > +     if (ret)
> > +             goto err;
> > +
> > +     val = (port << VSC73XX_MACACCESS_DEST_IDX_MASK_SHIFT) &
> > +           VSC73XX_MACACCESS_DEST_IDX_MASK;
> > +
> > +     vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> > +                         VSC73XX_MACACCESS,
> > +                         VSC73XX_MACACCESS_VALID |
> > +                         VSC73XX_MACACCESS_CMD_MASK |
> > +                         VSC73XX_MACACCESS_DEST_IDX_MASK |
> > +                         VSC73XX_MACACCESS_LOCKED,
> > +                         VSC73XX_MACACCESS_VALID |
> > +                         VSC73XX_MACACCESS_CMD_LEARN |
> > +                         VSC73XX_MACACCESS_LOCKED | val);
> > +     ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> > +
> > +err:
> > +     mutex_unlock(&vsc->fdb_lock);
> > +     return ret;
> > +}
> > +
> > +static int vsc73xx_fdb_add(struct dsa_switch *ds, int port,
> > +                        const unsigned char *addr, u16 vid, struct dsa_db db)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +
> > +     if (!vid) {
> > +             switch (db.type) {
> > +             case DSA_DB_PORT:
> > +                     vid = dsa_tag_8021q_standalone_vid(db.dp);
> > +                     break;
> > +             case DSA_DB_BRIDGE:
> > +                     vid = dsa_tag_8021q_bridge_vid(db.bridge.num);
>
> I appreciate the intention, but if you don't set ds->fdb_isolation
> (which you don't, although I believe the driver satisfies the documented
> requirements), db.bridge.num will always be passed as 0 in the
> .port_fdb_add() and .port_fdb_del() methods. Thus, dsa_tag_8021q_bridge_vid(0)
> will always be different than what dsa_tag_8021q_bridge_join() selects
> as VLAN-unaware bridge PVID for your ports. The FDB entry will be in a
> different VLAN than what your switch classifies the packets to. This
> means it won't match.
>
> Assuming this went through a reasonable round of testing (add bridge FDB
> entry towards expected port, make sure it isn't sent to others) and this
> issue was not noticed, then maybe the switch performs shared VLAN learning?
> Case in which, if you can't configure it to independent VLAN learning,
> it does not pass the ds->fdb_isolation requirements, plus the entire
> dance of picking a proper VID is pointless, as any chosen VID would have
> the same behavior.
>

ds->fdb_isolation was missed in this patch accidentally. Vsc73xx has
enabled independent learning by default.

> > +                     break;
> > +             default:
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     }
> > +
> > +     return vsc73xx_fdb_add_entry(vsc, port, addr, vid);
> > +}
> > +
> > +static int vsc73xx_fdb_del(struct dsa_switch *ds, int port,
> > +                        const unsigned char *addr, u16 vid, struct dsa_db db)
> > +{
> > +     struct vsc73xx *vsc = ds->priv;
> > +
> > +     if (!vid) {
> > +             switch (db.type) {
> > +             case DSA_DB_PORT:
> > +                     vid = dsa_tag_8021q_standalone_vid(db.dp);
> > +                     break;
> > +             case DSA_DB_BRIDGE:
> > +                     vid = dsa_tag_8021q_bridge_vid(db.bridge.num);
> > +                     break;
> > +             default:
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     }
> > +
> > +     return vsc73xx_fdb_del_entry(vsc, port, addr, vid);
> > +}
> > +
> > +static int vsc73xx_port_fdb_dump(struct dsa_switch *ds,
> > +                              int port, dsa_fdb_dump_cb_t *cb, void *data)
> > +{
> > +     struct vsc73xx_fdb fdb[VSC73XX_NUM_BUCKETS];
> > +     struct vsc73xx *vsc = ds->priv;
> > +     u16 i, bucket;
> > +
> > +     mutex_lock(&vsc->fdb_lock);
> > +
> > +     for (i = 0; i < VSC73XX_NUM_FDB_RECORDS; i++) {
> > +             vsc73xx_port_read_mac_table_entry(vsc, i, fdb);
> > +
> > +             for (bucket = 0; bucket < VSC73XX_NUM_BUCKETS; bucket++) {
> > +                     if (!fdb[bucket].valid || fdb[bucket].port != port)
> > +                             continue;
> > +
> > +                     /* We need to hide dsa_8021q VLANs from the user */
> > +                     if (vid_is_dsa_8021q(fdb[bucket].vid))
> > +                             fdb[bucket].vid = 0;
> > +                     cb(fdb[bucket].mac, fdb[bucket].vid, false, data);
>
> "cb" is actually dsa_user_port_fdb_do_dump(). It can return -EMSGSIZE
> when the netlink skb is full, and it is very important that you
> propagate that to the caller:
>
>         err = cb();
>         if (err)
>                 goto unlock;
>
> otherwise, you might notice that large FDB dumps will have missing FDB entries.
>
> > +             }
> > +     }
> > +
> > +     mutex_unlock(&vsc->fdb_lock);
> > +     return 0;
> > +}
> > +
> >  static const struct phylink_mac_ops vsc73xx_phylink_mac_ops = {
> >       .mac_config = vsc73xx_mac_config,
> >       .mac_link_down = vsc73xx_mac_link_down,
> > @@ -1851,6 +2148,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> >       .port_bridge_join = dsa_tag_8021q_bridge_join,
> >       .port_bridge_leave = dsa_tag_8021q_bridge_leave,
> >       .port_change_mtu = vsc73xx_change_mtu,
> > +     .port_fdb_add = vsc73xx_fdb_add,
> > +     .port_fdb_del = vsc73xx_fdb_del,
> > +     .port_fdb_dump = vsc73xx_port_fdb_dump,
> >       .port_max_mtu = vsc73xx_get_max_mtu,
> >       .port_stp_state_set = vsc73xx_port_stp_state_set,
> >       .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> > @@ -1981,6 +2281,8 @@ int vsc73xx_probe(struct vsc73xx *vsc)
> >               return -ENODEV;
> >       }
> >
> > +     mutex_init(&vsc->fdb_lock);
> > +
> >       eth_random_addr(vsc->addr);
> >       dev_info(vsc->dev,
> >                "MAC for control frames: %02X:%02X:%02X:%02X:%02X:%02X\n",
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> > index 3ca579acc798..a36ca607671e 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx.h
> > +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> > @@ -45,6 +45,7 @@ struct vsc73xx_portinfo {
> >   * @vlans: List of configured vlans. Contains port mask and untagged status of
> >   *   every vlan configured in port vlan operation. It doesn't cover tag_8021q
> >   *   vlans.
> > + * @fdb_lock: Mutex protects fdb access
> >   */
> >  struct vsc73xx {
> >       struct device                   *dev;
> > @@ -57,6 +58,7 @@ struct vsc73xx {
> >       void                            *priv;
> >       struct vsc73xx_portinfo         portinfo[VSC73XX_MAX_NUM_PORTS];
> >       struct list_head                vlans;
> > +     struct mutex                    fdb_lock; /* protects fdb access */
>
> Redundant comment since it's already in the kernel-doc?
>
> >  };
> >
> >  /**
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ