netdev - Re: Isolating DSA Slave Interfaces on a Bridge with Bridge Offloading

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220313133228.iff4tbkod7fmjgqn@skbuf>
Date:   Sun, 13 Mar 2022 15:32:28 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arınç ÜNAL <arinc.unal@...nc9.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Alvin Šipraga <ALSI@...g-olufsen.dk>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        DENG Qingfang <dqfext@...il.com>, erkin.bozoglu@...ont.com,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: Isolating DSA Slave Interfaces on a Bridge with Bridge Offloading

Hello Arınç,

On Sun, Mar 13, 2022 at 02:23:47PM +0300, Arınç ÜNAL wrote:
> Hi all,
> 
> The company I work with has got a product with a Mediatek MT7530 switch.
> They want to offer isolation for the switch ports on a bridge. I have run a
> test on a slightly modified 5.17-rc1 kernel. These commands below should
> prevent communication between the two interfaces:
> 
> bridge link set dev sw0p4 isolated on
> 
> bridge link set dev sw0p3 isolated on
> 
> However, computers connected to each of these ports can still communicate
> with each other. Bridge TX forwarding offload is implemented on the MT7530
> DSA driver.
> 
> What I understand is isolation works on the software and because of the
> bridge offloading feature, the frames never reach the CPU where we can block
> it.
> 
> Two solutions I can think of:
> 
> - Disable bridge offloading when isolation is enabled on a DSA slave
> interface. Not the best solution but seems easy to implement.
> 
> - When isolation is enabled on a DSA slave interface, do not mirror the
> related FDB entries to the switch hardware so we can keep the bridge
> offloading feature for other ports.
> 
> I suppose this could only be achieved on switch specific DSA drivers so the
> implementation would differ by each driver.
> 
> Cheers.
> Arınç

To be clear, are you talking about a patched or unpatched upstream
kernel? Because mt7530 doesn't implement bridge TX forwarding offload.
This can be seen because it is missing the "*tx_fwd_offload = true;"
line from its ->port_bridge_join handler (of course, not only that).

You are probably confused as to why is the BR_ISOLATED brport flag is
not rejected by a switchdev interface when it will only lead to
incorrect behavior.

In fact we've had this discussion with Qingfang, who sent this patch to
allow switchdev drivers to *at the very least* reject the flag, but
didn't want to write up a correct commit description for the change:
https://lore.kernel.org/all/CALW65jbotyW0MSOd-bd1TH_mkiBWhhRCQ29jgn+d12rXdj2pZA@mail.gmail.com/T/
https://patchwork.kernel.org/project/netdevbpf/patch/20210811135247.1703496-1-dqfext@gmail.com/
https://patchwork.kernel.org/project/netdevbpf/patch/20210812142213.2251697-1-dqfext@gmail.com/

As a result, currently not even the correctness issue has still not yet
been fixed, let alone having any driver act upon the feature correctly.

In my opinion, it isn't mandatory that bridge ports with BR_ISOLATED
have forwarding handled in software. All that needs to change is that
their forwarding domain, as managed by the ->port_bridge_join and
->port_bridge_leave callbacks, is further restricted. Isolated ports can
forward only to non-isolated ports, and non-isolated ports cannot
forward to isolated ports. Things may become more interesting with
bridge TX forwarding offload though, but as I mentioned, at least
upstream this isn't a concern for mt7530 (yet).