netdev - Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

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]

Date:   Sat, 14 Nov 2020 13:36:25 +0100
From:   "Tobias Waldekranz" <tobias@...dekranz.com>
To:     "Vladimir Oltean" <olteanv@...il.com>
Cc:     "Andrew Lunn" <andrew@...n.ch>, <davem@...emloft.net>,
        <kuba@...nel.org>, <vivien.didelot@...il.com>,
        <f.fainelli@...il.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and
 ethertype DSA taggers

On Sat Nov 14, 2020 at 3:20 PM CET, Vladimir Oltean wrote:
> On Sat, Nov 14, 2020 at 12:29:32PM +0100, Tobias Waldekranz wrote:
> > > Humm, yes, they have not been forwarded by hardware. But is the
> > > software bridge going to do the right thing and not flood them? Up
> >
> > The bridge is free to flood them if it wants to (e.g. IGMP/MLD
> > snooping is off) or not (e.g. IGMP/MLD snooping enabled). The point
> > is, that is not for a lowly switchdev driver to decide. Our job is to
> > relay to the bridge if this skb has been forwarded or not, the end.
> >
> > > until now, i think we did mark them. So this is a clear change in
> > > behaviour. I wonder if we want to break this out into a separate
> > > patch? If something breaks, we can then bisect was it the combining
> > > which broke it, or the change of this mark.
> >
> > Since mv88e6xxx can not configure anything that generates
> > DSA_CODE_MGMT_TRAP or DSA_CODE_POLICY_TRAP yet, we do not have to
> > worry about any change in behavior there.
> >
> > That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:
> >
> > Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
> > packets, whereas tag_edsa.c will exempt them. So we can not unify the
> > two without changing the behavior of one.
> >
> > I posit that tag_edsa does the right thing, the packet has not been
> > forwarded, so we should go with that.
> >
> > This is precisely the reason why we want to unify these! :)
>
> Shouldn't the correct approach be to monitor the
> SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute in order to figure out
> whether IGMP/MLD snooping is enabled or not?

The correct thing is to do both.

Independent of wheter IGMP/MLD snooping is statically or dynamically
enabled, it is always true that a TO_CPU with code 2 has _never_ been
forwarded in hardware and a FORWARD _always_ has been (which is the
tag you would see on IGMP frames reaching the CPU in that case).

In the static case, we would always get TO_CPUs with code 2, and thus
never set offload_fwd_mark. If snooping is enabled on the bridge, they
won't be forwarded. If snooping is disabled, the bridge will want to
forward, which it can since the mark is not set.

In the dynamic case, we would get TO_CPUs with code 2 when snooping is
enabled. The mark won't be set, but the bridge does not want to
forward anyway. When snooping is disabled, we would get FORWARDs. The
bridge will want to forward those, see that the mark is set, and skip
it.

The only combination that does not work is of course to have snooping
permanently _off_ in hardware and expect to use snooping correctly on
the software bridge. In that case you would always get FORWARDs, and
always set the mark. The bridge will not want to flood, but it is too
late since the hardware has already done it.