netdev - Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports

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: <20220411131619.43js6owwkalcdwwa@skbuf>
Date:   Mon, 11 Apr 2022 16:16:19 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload
 of matchall for bridged DSA ports

On Mon, Apr 11, 2022 at 02:48:29PM +0200, Mattias Forsblad wrote:
> My thinking was that the notifier I was aware of was the one I implemented
> and someway was a preparation for consumers (that didn't exist yes). I didn't even
> know about dsa_tree_notify(). So I'll remove that part, yes? Is that ok even
> if it's not optimal, like you say?

Optimal or sub-optimal, I think there are bigger problems which we're
taking too lightly. Like this one:

| If there is tc rules on a bridge and all the ports leave the bridge
| and then joins the bridge again, the indirect framwork doesn't seem
| to reoffload them at join. The tc rules need to be torn down and
| re-added. This seems to be because of limitations in the tc
| framework. A fix for this would need another patch-series in itself.
| However we prepare for when this issue is fixed by registring and
| deregistring when a dsa_bridge is created/destroyed so it should
| work when it's solved.

You've presented just the more convenient angle of the limitation
(DSA interfaces present as bridge ports, then leave, then re-join).
But the same problem is there even when the tc rule is added to the
bridge before adding any port to it. Which is more likely to result in
buggy user experience, because it doesn't say anywhere that there are
restrictions to the order in which things should be set up.

Personally, I would first try to ask for help, as some work was clearly
put into the indirect flow block offload, and the reasonable expectation
is that the filter replay (and not just the bind/unbind) works on
register/unregister, yet for some unknown reason it doesn't.

Then, if I get no answer/help, I would consider as an alternative
implementing the flow block binding logic in the bridge, and
re-notifying the relevant stuff through switchdev. Via this
SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV we're talking about - hence the
reason I'm mentioning it. After all, the flow block binding code is
mostly bloatware, so we can reduce the duplicated code switchdev drivers
have to write.

But I wouldn't consider leaving this up in the air if there is a
non-complicated way to address it, which it seems like there is.

So in short, let's discuss what's optimal overall only when we see an
implementation that fully works, ok? It was my mistake to point out
during review minor optimizations that could be made even if they don't
follow the overall direction that the patch set should go in.