netdev - Re: [PATCH net-next 0/6] Disable host flooding for DSA ports under a bridge

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: <20220411142402.vhuctrmlyezklg4n@skbuf>
Date:   Mon, 11 Apr 2022 17:24:02 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Jiri Pirko <jiri@...dia.com>, Ido Schimmel <idosch@...dia.com>,
        Mattias Forsblad <mattias.forsblad@...il.com>,
        Joachim Wiberg <troglobit@...il.com>
Subject: Re: [PATCH net-next 0/6] Disable host flooding for DSA ports under a
 bridge

On Mon, Apr 11, 2022 at 04:55:30PM +0300, Vladimir Oltean wrote:
> > > IFF_ALLMULTI would implicitly correspond to BR_MCAST_FLOOD (ok, also
> > > adjust for mcast_router ports).
> > 
> > IFF flags shouldn't correspond to any bridge flags. They describe higher
> > level features. Therefore, they should be passed down to the drivers,
> > who in many cases may decide to use hardware resources that are shared
> > with bridge flags (i.e. flood controls), but in some cases may be able
> > to do something better.
> > 
> > As an example of "something better": some ASICs have separate flooding
> > controls for IP and non-IP multicast.
> > 
> > So, I think
> > 
> >     ip link set dev br0 allmulticast on
> > 
> > Should be one way for userspace to tell the bridge to mark the host port
> > as a permanent multicast router port. This in turn would trigger a
> > 
> >     switchdev_port_attr_set(dev,
> >     	&{ .id = SWITCHDEV_ATTR_ID_BRIDGE_MROUTER ... }, extack);
> > 
> > At the DSA layer this info would be passed to the driver, which will
> > decide if that means the same thing as BR_MCAST_FLOOD or something
> > else.
> 
> Yeah, I don't think so. Doing nothing at all is way better than
> entangling the RX filtering logic even more with the forwarding logic,
> IMHO.

Thinking out loud.
I still maintain it is weird and uncalled for if "ip link set dev br0 allmulticast on"
marks the bridge as a multicast router (maybe I'm wrong, but I don't see why it is helpful).
But the other way around may not be so weird. That is, as long as the
bridge is a multicast router port or needs to receive unknown multicast
for any reason at all, it auto-enables IFF_ALLMULTI on its RX flags.

Then, we just need to take care of that "RX filter is a mirror" thing.
Not ideal, but maybe if we actually introduce a bridge flag "rx_filter_mirror"
where the default is 1 to keep backwards compatibility, we could actually
turn the bridge into something sane?
The bad part is that we can't make switchdev drivers reject a bridge
with rx_filter_mirror=1, unless we're ready to deal with the breakage
(although even that is tempting...).

What this effort would achieve is that the bridge would no longer become
promiscuous in the presence of uppers with different MAC address.
It would do this by implementing the filtering you talked about in
br_pass_frame_up().

As for offloading, the nice part is that the bridge RX filtering logic
could be left as invisible to switchdev, and we could concentrate only
on flooding towards the host via the logic that Joachim is working on.