Date:   Sun, 19 Apr 2020 15:47:01 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     "Allan W. Nielsen" <allan.nielsen@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Antoine Tenart <antoine.tenart@...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>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, Po Liu <po.liu@....com>,
        Jiri Pirko <jiri@...lanox.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] net: mscc: ocelot: deal with problematic
 MAC_ETYPE VCAP IS2 rules

Hi Ido, Allan,

On Sun, 19 Apr 2020 at 11:30, Ido Schimmel <idosch@...sch.org> wrote:
>
> On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
> > Hi,
> >
> > Sorry I did not manage to provide feedback before it was merged (I will
> > need to consult some of my colleagues Monday before I can provide the
> > foll feedback).
> >
> > There are many good things in this patch, but it is not only good.
> >
> > The problem is that these TCAMs/VCAPs are insanely complicated and it is
> > really hard to make them fit nicely into the existing tc frame-work
> > (being hard does not mean that we should not try).
> >
> > In this patch, you try to automatic figure out who the user want the
> > TCAM to be configured. It works for 1 use-case but it breaks others.
> >
> > Before this patch you could do a:
> >     tc filter add dev swp0 ingress protocol ipv4 \
> >             flower skip_sw src_ip 10.0.0.1 action drop
> >     tc filter add dev swp0 ingress \
> >             flower skip_sw src_mac 96:18:82:00:04:01 action drop
> >
> > But the second rule would not apply to the ICMP over IPv4 over Ethernet
> > packet, it would however apply to non-IP packets.
> >
> > With this patch it not possible. Your use-case is more common, but the
> > other one is not unrealistic.
> >
> > My concern with this, is that I do not think it is possible to automatic
> > detect how these TCAMs needs to be configured by only looking at the
> > rules installed by the user. Trying to do this automatic, also makes the
> > TCAM logic even harder to understand for the user.
> >
> > I would prefer that we by default uses some conservative default
> > settings which are easy to understand, and then expose some expert
> > settings in the sysfs, which can be used to achieve different
> > behavioral.
> >
> > Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> > understand default.
> >
> > But I do seem to recall that there is a way to allow matching on both
> > SMAC and SIP (your original motivation). This may be a better default
> > (despite that it consumes more TCAM resources). I will follow up and
> > check if this is possible.
> >
> > Vladimir (and anyone else whom interested): would you be interested in
> > spending some time discussion the more high-level architectures and
> > use-cases on how to best integrate this TCAM architecture into the Linux
> > kernel. Not sure on the outlook for the various conferences, but we
> > could arrange some online session to discuss this.
>
> Not sure I completely understand the difficulties you are facing, but it
> sounds similar to a problem we had in mlxsw. You might want to look into
> "chain templates" [1] in order to restrict the keys that can be used
> simultaneously.
>
> I don't mind participating in an online discussion if you think it can
> help.
>
> [1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates

I think it is worth giving a bit of context on what motivated me to
add this patch. Luckily I believe I can summarize it in a paragraph
below.

I am trying to understand practical ways in which IEEE 802.1CB can be
used - an active redundancy mechanism similar to HSR/PRP which relies
on sending sequence-numbered frame duplicates and eliminating those
duplicates at the destination (as opposed to passive redundancy
mechanisms such as RSTP, MRP etc which rely on BLOCKING port states to
stop L2 forwarding loops from killing the network). So since 802.1CB
needs a network where none of the port states can be put to BLOCKING
(as that would break the forwarding of some of the replicated
streams), I need a way to limit the impact of L2 loops. Currently I am
using, rather successfully, an idea borrowed from HSR called
"self-address filtering". It says that received packets can be dropped
if their source MAC address matches the device's MAC address. This
feature is useful for ensuring that packets never traverse a ring
network more than once.
To implement this idea, I use an offloaded tc-flower rule matching on
src_mac with an action of "drop".

To my surprise, such a src_mac rule does not do what's written on the
box with the Ocelot switch flow classification engine called VCAP IS2.
That is, packets having that src_mac would only get dropped if their
protocol is not ARP, SNAP, IPv4, IPv6 and maybe others. Clearly such a
rule is less than useful for the purpose we want it to.
I did raise this concern here, and the suggestion that I got is to
implement something like this patch, aka enable a port setting which
forces matches on MAC_ETYPE keys only, regardless of higher-layer
protocol information:
https://lkml.org/lkml/2020/2/24/489
So the default (pre-patch) behavior is for IP (and other) matches to
be sane, at the expense of MAC matches being insane.
Whereas the current behavior is for MAC matches to be sane, at the
expense of IP matches becoming impossible as long as MAC rules are
also present.
In this context, Allan's complaint seems to be that the MAC matches
were "good enough" for them, even if not all MAC address matches were
caught, at least it did not prevent them from installing IP matching
rules on the same port.

I may not have completely understood Ido's suggestion to use
FLOW_CLS_TMPLT_CREATE (I might lack the imagination of how it can be
put to practical use to solve the clash here), but I do believe that
it is only a way for the driver to eliminate the guesswork out of the
user's intention.
In this case, my personal opinion is that the intention is absolutely
clear: a classifier with src_mac should match on all frames having
that src_mac (if that is not commonly agreed here, a good rule of
thumb is to compare with what a non-offloaded tc filter rule does).
Whereas the "non-problematic" MAC matches that the VCAP IS2 _is_ able
to still perform [ without calling ocelot_match_all_as_mac_etype ]
should be expressed in terms of a more specific classification key to
tc, such as:

tc filter add dev swp0 ingress *protocol 0x88f7* flower src_mac
96:18:82:00:04:01 action drop

In the above case, because "protocol" is not ipv4, ipv6, arp, snap,
then these rules can happily live together without ever needing to
call ocelot_match_all_as_mac_etype. If we agree on this solution, I
can send a patch that refines the ocelot_ace_is_problematic_mac_etype
function.

Thanks,
-Vladimir

Powered by blists - more mailing lists