netdev - Re: [PATCH net] net: dsa: reference count the host mdb addresses

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]

Date:   Sat, 28 Nov 2020 10:34:13 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
        "kuba\@kernel.org" <kuba@...nel.org>,
        "andrew\@lunn.ch" <andrew@...n.ch>,
        "f.fainelli\@gmail.com" <f.fainelli@...il.com>,
        "vivien.didelot\@gmail.com" <vivien.didelot@...il.com>
Subject: Re: [PATCH net] net: dsa: reference count the host mdb addresses

On Sat, Nov 28, 2020 at 00:27, Vladimir Oltean <vladimir.oltean@....com> wrote:
> On Sat, Nov 28, 2020 at 12:58:10AM +0100, Tobias Waldekranz wrote:
>> That sounds like a good idea. We have run into another issue with the
>> MDB that maybe could be worked into this changeset. This is what we have
>> observed on 4.19, but from looking at the source it does not look like
>> anything has changed with respect to this issue.
>>
>> The DSA driver handles the addition/removal of router ports by
>> enabling/disabling multicast flooding to the port in question. On
>> mv88e6xxx at least, this is only part of the solution. It only takes
>> care of the unregistered multicast. You also have to iterate through all
>> _registered_ groups and add the port to the destination vector.
>
> And this observation is based on what? Based on this paragraph from RFC4541?

Well in all honesty, it is mostly based on information from a colleague
of mine who knows the ins and outs of all these RFCs.

> 2.1.2.  Data Forwarding Rules
>
>    1) Packets with a destination IP address outside 224.0.0.X which are
>       not IGMP should be forwarded according to group-based port
>       membership tables and must also be forwarded on router ports.

...but yes, that paragraph does not leave a lot of wiggle room :)

> Let me ask you a different question. Why would DSA be in charge of
> updating the MDB records, and not the bridge? Or why DSA and not the end
> driver? Ignore my patch. I'm just trying to understand what you're
> saying. Why precisely DSA, the mid layer? I don't know, this is new
> information to me, I'm still digesting it.

The bridge has all the necessary information for sure. But it has a
different model with a separate list of router ports. Then in
br_multicast_flood you simply forward to the union of the group entry
and the router ports. It is not the bridge's fault that our hardware
does not have a separate bitmask for router ports. Some hardware may
very well have it.

I guess we could create internal APIs to the bridge to retrieve the
information though. There is already `bool br_multicast_router(dev)`, so
we should only need to add `bool br_multicast_member(dev, group, vid)`.

Assuming that those were available we should be able to solve it either
at the DSA or the driver layer. I seem to recall some issue that forced
us to place the cache at the dst level, but I would have to go through
the implementation to figure out what that issue was.