Re: [RFC net-next] net: tc: flow indirect framework issue

From: Mattias Forsblad <mattias.forsblad@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>,
	Baowen Zheng <baowen.zheng@corigine.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"roid@nvidia.com" <roid@nvidia.com>,
	"vladbu@nvidia.com" <vladbu@nvidia.com>,
	Eli Cohen <elic@nvidia.com>, Jiri Pirko <jiri@resnulli.us>,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	Tobias Waldekranz <tobias@waldekranz.com>
Subject: Re: [RFC net-next] net: tc: flow indirect framework issue
Date: Wed, 13 Apr 2022 14:24:38 +0200	[thread overview]
Message-ID: <2a82cf39-48b9-2c6c-f662-c1d1bce391ba@gmail.com> (raw)
In-Reply-To: <20220413090705.zkfrp2fjhejqdj6a@skbuf>

On 2022-04-13 11:07, Vladimir Oltean wrote:
> Hi Baowen,
> 
> On Wed, Apr 13, 2022 at 07:05:39AM +0000, Baowen Zheng wrote:
>> Hi Mattias, in my understand, your problem may because you register
>> your callback here. I am not sure why you choose to register your hook
>> here(after the interface is added to the bridge to just trigger the
>> callback in necessary case.)
> 
>> Then your function is called and add your cb to the tcf_block. But
>> since the matchall filter has been created so you can not get your
>> callback triggered. 
> 
> The bridge device has a certain lifetime, and the physical port device
> has a different and completely independent lifetime. So the port device
> may join the bridge when the bridge already has some filters on its
> ingress chain.
> 
> Even with the indirect flow block callback registered at module load
> stage as you suggest, the port driver needs to have a reason to
> intercept a tc filter on a certain bridge. And when the port isn't a
> member (yet) of that bridge, it has no reason to.
> 
> Of course, you could make the port driver speculatively monitor all tc
> filters installed on all bridges in the system (related or unrelated to
> ports belonging to this driver), just to not miss callbacks which may be
> needed later. But that is quite suboptimal.
> 
> Mattias' question comes from the fact that there is already some logic
> in flow_indr_dev_register() to replay missed flow block binding events,
> added by Eli Cohen in commit 74fc4f828769 ("net: Fix offloading indirect
> devices dependency on qdisc order creation"). That logic works, but it
> replays only the binding, not the actual filters, which again, would be
> necessary.
> 
>> Maybe you can try to regist your callback in your module load stage I
>> think your callback will be triggered, or change the command order as: 
>> tc qdisc add dev br0 clsact
>> ip link set dev swp0 master br0
>> tc filter add dev br0 ingress pref 1 proto all matchall action drop
>> I am not sure whether it will take effect.
> 
> I think the idea is to make the given command order work, not to change it.

Re-ordering the tc commands doesn't solve the issue when all ports leave the
bridge, which will lead to flow_indr_dev_unregister() and later re-joins
the bridge (flow_indr_dev_register()). We'll need filter replay for this.