netdev - Re: [PATCH net-next] net: mscc: ocelot: Workaround to allow traffic to CPU in standalone mode

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: <20200219101149.dq7jwhs6aypv43kf@lx-anielsen.microsemi.net>
Date:   Wed, 19 Feb 2020 11:11:49 +0100
From:   "Allan W. Nielsen" <allan.nielsen@...rochip.com>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Andrew Lunn <andrew@...n.ch>,
        "Florian Fainelli" <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        netdev <netdev@...r.kernel.org>,
        "Microchip Linux Driver Support" <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next] net: mscc: ocelot: Workaround to allow traffic
 to CPU in standalone mode

On 18.02.2020 16:02, Vladimir Oltean wrote:
>The problem is on RX.
>
>> Is it with the broadcast ARP, or is it the following unicast packet?
>For the unicast packet.
When you have it working (in your setup, with your patch applied). Does
the ping reply packet have an IFH (DSA-tag)?

Or is it a frame on the NPI port without an IFH.

This is important as this will tell us of the frame was copied to CPU
and then redirected to the NPI port, or if it was plain forwarded.

I need to understand the problem better before trying to solve it.

>> >But if I do this:
>> >ip link add dev br0 type bridge
>> >ip link set dev swp0 master br0
>> >ip link set dev swp0 nomaster
>> >ping 192.168.1.2
>> >Then it works, because the code path from ocelot_bridge_stp_state_set
>> >that puts the CPU port in the forwarding mask of the other ports gets
>> >executed on the "bridge leave" action.
>> >The whole point is to have the same behavior at probe time as after
>> >removing the ports from the bridge.
>> This does sound like a bug, but I still do not agree in the solution.
>>
>> >The code with ocelot_mact_learn towards PGID_CPU for the MAC addresses
>> >of the switch port netdevices is all bypassed in Felix DSA. Even if it
>> >weren't, it isn't the best solution.
>> >On your switch, this test would probably work exactly because of that
>> >ocelot_mact_learn.
>> So I guess it is the reception of the unicast packet which is causing
>> problems.
>>
>> >But try to receive packets sent at any other unicast DMAC immediately
>> >after probe time, and you should see them in tcpdump but won't.
>> That is true - this is because we have no way of implementing promisc
>> mode, which still allow us to HW offload of the switching. We discussed
>> this before.
>>
>> Long story short, it sounds like you have an issue because the
>> Felix/DSA driver behave differently than the Ocelot. Could you try to do
>> your fix such that it only impact Felix and does not change the Ocelot
>> behavioral.
>
>It looks like you disagree with having BIT(ocelot->cpu) in PGID_SRC +
>p (the forwarding matrix) and just want to rely on whitelisting
>towards PGID_CPU*?
Yes.

When the port is not member of the bridge, it should act as a normal NIC
interface.

With this change frames are being forwarded even when the port is not
member of the bridge. This may be what you want in a DSA (or may not -
not sure), but it is not ideal in the Ocelot/switchdev solution as we
want to use the MAC-table to do the RX filtering.

>But you already have that logic present in your driver, it's just not
>called from a useful place for Felix.
>So it logically follows that we should remove these lines from
>ocelot_bridge_stp_state_set, no?
>
>            } else {
>                    /* Only the CPU port, this is compatible with link
>                     * aggregation.
>                     */
>                    ocelot_write_rix(ocelot,
>                                     BIT(ocelot->cpu),
>                                     ANA_PGID_PGID, PGID_SRC + p);
This should not be removed. When the port is member of the bridge this
bit must be set. When it is removed it must be cleared again.

>*I admit that I have no idea why it works for you, and why the frames
>learned towards PGID_CPU are forwarded to the CPU _despite_
>BIT(ocelot->cpu) not being present in PGID_SRC + p.
I believe this is because we have the MAC address in the MAC table.

It seems that you want to use learning to forward frames to the CPU,
also in the case when the port is not a member of the bridge. I'm not
too keen on this, mainly because I'm not sure how well it will work. If
you are certain this is what you want for Felix then lets try find a way
to make it happend for Felix without chancing the behaivural for Ocelot.

An alternative solution would be to use the MAC-table for white listing
of unicast packets. But as I understand the thread this is not so easy
to do with DSA. Sorry, I do not know DSA very well, and was not able to
fully understand why. But this is as far as I know the only way to get
the proper RX filtering.

An other solution, is to skip the RX filtering, and when a port is not
member of a beidge set the 'ANA:PORT[0-11]:CPU_FWD_CFG.CPU_SRC_COPY_ENA'
bit. This will cause all fraems to be copied to the CPU. Again, we need
to find a way to do this which does not affect Ocelot.

/Allan