linux-kernel - Re: [PATCH net-next v1 2/2] net: dsa: microchip: Add partial ACL support for ksz9477 switches

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: <20230418045526.GB30964@pengutronix.de>
Date:   Tue, 18 Apr 2023 06:55:26 +0200
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Petr Machata <petrm@...dia.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        UNGLinuxDriver@...rochip.com, Eric Dumazet <edumazet@...gle.com>,
        kernel@...gutronix.de, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v1 2/2] net: dsa: microchip: Add partial ACL
 support for ksz9477 switches

On Mon, Apr 17, 2023 at 03:19:17PM +0300, Vladimir Oltean wrote:
> On Mon, Apr 17, 2023 at 01:03:11PM +0200, Oleksij Rempel wrote:
> > Certain aspects of the chip specification appeared ambiguous, leading me
> > to decide to allocate a separate time slot for investigating the counter
> > topic if necessary.
> > 
> > For example, according to the
> > KSZ9477 4.4.18 ACCESS CONTROL LIST (ACL) FILTERING:
> > 
> > "It is also possible to configure the ACL table so that multiple processing
> > entries specify the same action rule. In this way, the final matching result is
> > the OR of the matching results from each of the multiple RuleSets.
> > The 16 ACL rules represent an ordered list, with entry #0 having the highest
> > priority and entry #15 having the lowest priority. All matching rules are
> > evaluated. If there are multiple true match results and multiple corresponding
> > actions, the highest priority (lowest numbered) of those actions will be the
> > one taken."
> > 
> > A summary of this part of documentation is:
> > 1. ACL table can have multiple entries specifying the same action rule.
> > 2. Final matching result is the OR of multiple RuleSets' results.
> > 3. 16 ACL rules form an ordered list, with priority descending from #0 to #15.
> > 4. All matching rules are evaluated.
> > 5. When multiple true matches and actions occur, the highest priority action is
> >    executed.
> > 
> > Considering this, there is a possibility that separate action rules would not
> > be executed, as they might not be the highest priority match.  Since counters
> > would have separation action rules, they would not be executed or prevent other
> > action rules from execution.
> > 
> > To confirm my hypothesis, additional time and testing will be required.
> > Nonetheless, I hope this issue does not impede the progress of this patch.
> 
> This is the kind of stuff you'd have to know when adding a software model
> for the rules, right?

right :)

> Could you consider writing a selftest that
> precisely illustrates the matching pattern of the hardware? It would be
> good if the same test could then be run on a software-only implementation
> and if the behavior would match. The tc tool should be more than a
> vendor agnostic tool of doing vendor specific stuff. It should offload
> as faithfully as possible the software data path. It would also be good,
> but I haven't studied or used this test personally, if the test could be
> based on the existing tools/testing/selftests/net/forwarding/skbedit_priority.sh.

ok I added it to my todo list. My next time slot for this project will
be in two months.

> > > > > Have you considered the "skbedit priority" action as opposed to hw_tc?
> > > > 
> > > > I had already thought of that, but since bridging is offloaded in the HW
> > > > no skbs are involved, i thought it will be confusing. Since tc-flower seems to
> > > > already support hw_tc remapping, I decided to use it. I hope it will not harm,
> > > > to use it for now as mandatory option and make it optional later if other
> > > > actions are added, including skbedit.
> > > 
> > > Well, skbedit is offloadable, so in that sense, its behavior is defined
> > > even when no skbs are involved. OTOH, skbedit also has a software data
> > > path (sets skb->priority), as opposed to hw_tc, which last time I checked,
> > > did not.
> > 
> > Alright, having tc rules be portable is certainly a benefit. I presume
> > that in this situation, it's not an exclusive "either...or" choice. Both
> > variants can coexist, and the skbedit action can be incorporated at a
> > later time. Is that accurate?
> 
> I believe Petr Machata (now copied) could have an opinion here too.
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |