| 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
| ||
|
Message-ID: <20220201175527.mvzn4vstgbgmnijs@skbuf>
Date: Tue, 1 Feb 2022 19:55:27 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 4/5] net: dsa: mv88e6xxx: Improve multichip
isolation of standalone ports
On Mon, Jan 31, 2022 at 04:46:54PM +0100, Tobias Waldekranz wrote:
> Given that standalone ports are now configured to bypass the ATU and
> forward all frames towards the upstream port, extend the ATU bypass to
> multichip systems.
>
> Load VID 0 (standalone) into the VTU with the policy bit set. Since
> VID 4095 (bridged) is already loaded, we now know that all VIDs in use
> are always available in all VTUs. Therefore, we can safely enable
> 802.1Q on DSA ports.
>
> Setting the DSA ports' VTU policy to TRAP means that all incoming
> frames on VID 0 will be classified as MGMT - as a result, the ATU is
> bypassed on all subsequent switches.
>
> With this isolation in place, we are able to support configurations
> that are simultaneously very quirky and very useful. Quirky because it
> involves looping cables between local switchports like in this
> example:
>
> CPU
> | .------.
> .---0---. | .----0----.
> | sw0 | | | sw1 |
> '-1-2-3-' | '-1-2-3-4-'
> $ @ '---' $ @ % %
>
> We have three physically looped pairs ($, @, and %).
>
> This is very useful because it allows us to run the kernel's
> kselftests for the bridge on mv88e6xxx hardware.
>
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 63 ++++++++++++++++++++++----------
> 1 file changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 8896709b9103..d0d766354669 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1630,21 +1630,11 @@ static int mv88e6xxx_fid_map_vlan(struct mv88e6xxx_chip *chip,
>
> int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap)
> {
> - int i, err;
> - u16 fid;
> -
> bitmap_zero(fid_bitmap, MV88E6XXX_N_FID);
>
> - /* Set every FID bit used by the (un)bridged ports */
> - for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
> - err = mv88e6xxx_port_get_fid(chip, i, &fid);
> - if (err)
> - return err;
> -
> - set_bit(fid, fid_bitmap);
> - }
> -
> - /* Set every FID bit used by the VLAN entries */
> + /* Every FID has an associated VID, so walking the VTU
> + * will discover the full set of FIDs in use.
> + */
So practically, regardless of whether the switch supports VTU policy or
not, we still load VID 0 in the VTU, and this simplifies the driver a
bit. Could we also simplify mv88e6xxx_port_db_dump() by deleting the
mv88e6xxx_port_get_fid() from there (and then delete this function
altogether)?
I think the mv88e6xxx_port_set_fid() call is now useless too?
> return mv88e6xxx_vtu_walk(chip, mv88e6xxx_fid_map_vlan, fid_bitmap);
> }
>
> @@ -1657,10 +1647,7 @@ static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
> if (err)
> return err;
>
> - /* The reset value 0x000 is used to indicate that multiple address
> - * databases are not needed. Return the next positive available.
> - */
> - *fid = find_next_zero_bit(fid_bitmap, MV88E6XXX_N_FID, 1);
> + *fid = find_first_zero_bit(fid_bitmap, MV88E6XXX_N_FID);
> if (unlikely(*fid >= mv88e6xxx_num_databases(chip)))
> return -ENOSPC;
>
> @@ -2152,6 +2139,9 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
> if (!vlan.valid) {
> memset(&vlan, 0, sizeof(vlan));
>
> + if (vid == MV88E6XXX_VID_STANDALONE)
> + vlan.policy = true;
> +
> err = mv88e6xxx_atu_new(chip, &vlan.fid);
> if (err)
> return err;
> @@ -2949,8 +2939,43 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> if (err)
> return err;
>
> + /* On chips that support it, set all DSA ports' VLAN policy to
> + * TRAP. In combination with loading MV88E6XXX_VID_STANDALONE
> + * as a policy entry in the VTU, this provides a better
> + * isolation barrier between standalone ports, as the ATU is
> + * bypassed on any intermediate switches between the incoming
> + * port and the CPU.
> + */
> + if (!dsa_is_user_port(ds, port) && chip->info->ops->port_set_policy) {
Will this not also affect FWD frames sent on behalf of VLAN-unaware
bridges as they are received on CPU ports and upstream-facing DSA ports?
Somehow I think you intend to make this match only on downstream-facing
DSA ports.
> + err = chip->info->ops->port_set_policy(chip, port,
> + MV88E6XXX_POLICY_MAPPING_VTU,
> + MV88E6XXX_POLICY_ACTION_TRAP);
> + if (err)
> + return err;
> + }
> +
> + /* User ports start out in standalone mode and 802.1Q is
> + * therefore disabled. On DSA ports, all valid VIDs are always
> + * loaded in the VTU - therefore, enable 802.1Q in order to take
> + * advantage of VLAN policy on chips that supports it.
> + */
Is this really needed? I thought cascade ports parse the VID from the
DSA header regardless of 802.1Q mode.
> err = mv88e6xxx_port_set_8021q_mode(chip, port,
> - MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED);
> + dsa_is_user_port(ds, port) ?
> + MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED :
> + MV88E6XXX_PORT_CTL2_8021Q_MODE_SECURE);
> + if (err)
> + return err;
> +
> + /* Bind MV88E6XXX_VID_STANDALONE to MV88E6XXX_FID_STANDALONE by
> + * virtue of the fact that mv88e6xxx_atu_new() will pick it as
> + * the first free FID. This will be used as the private PVID for
> + * unbridged ports. Shared (DSA and CPU) ports must also be
> + * members of this VID, in order to trap all frames assigned to
> + * it to the CPU.
> + */
> + err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_STANDALONE,
> + MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
> + false);
> if (err)
> return err;
>
> @@ -2963,7 +2988,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
> * relying on their port default FID.
> */
> err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED,
> - MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED,
> + MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
I think the idea with UNTAGGED here was that packets sent by tag_dsa.c
with TX forwarding offload on behalf of a VLAN-unaware bridge have VID
4095. By setting the port as untagged, that VID is stripped on egress.
If you make it UNMODIFIED, the outside world will see it. Or am I wrong?
> false);
> if (err)
> return err;
> --
> 2.25.1
>
Powered by blists - more mailing lists