Message-ID: <20200419135143.GA3487966@splinter>
Date:   Sun, 19 Apr 2020 16:51:43 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Vladimir Oltean <olteanv@...il.com>
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

On Sun, Apr 19, 2020 at 03:47:01PM +0300, Vladimir Oltean wrote:
> 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.

I was under the impression that you can't mix different keys (e.g.,
src_mac + src_ip), but now I understand that you can't mix different
keys in case of specific key *values*. This will work correctly:

$ tc filter add dev swp0 ingress proto ip \
	flower src_ip 192.0.2.1 action drop
$ tc filter add dev swp0 ingress proto 0x88f7 \
	flower src_mac 00:11:22:33:44:55 action drop

This will not work correctly:

$ tc filter add dev swp0 ingress proto ip \
	flower src_ip 192.0.2.1 action drop
$ tc filter add dev swp0 ingress proto all \
	flower src_mac 00:11:22:33:44:55 action drop

Correct? If so, I don't think the templates can help you. They are about
forcing only specific keys, regardless of value.

> 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).

I agree.

> 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:

Yes.

> 
> 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.

I think it makes sense. You are basically being explicit about the
hardware limitations and denying configurations that cannot always work.
Previous approach was to allow configurations that sometimes work.

Powered by blists - more mailing lists