Re: [PATCH net-next 2/2] net/sched: act_tunnel_key: skip dst cache for skip_sw actions

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch only saves the dst_cache when TCA_ACT_FLAGS_SKIP_SW
  is already in act_flags while tunnel_key_init() runs.

--

net/sched: act_tunnel_key: skip dst cache for skip_sw actions

This commit makes tunnel_key_init() skip dst_cache_init() when the
tunnel_key action is flagged skip_sw, because such an action never routes
packets. On replace it checks the flags of the existing action rather
than the flags in the request.

> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 5038348533060..28d9cc080101a 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -481,9 +481,13 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  		}
>  
>  #ifdef CONFIG_DST_CACHE
> -		ret = dst_cache_init(&metadata->u.tun_info.dst_cache, GFP_KERNEL);
> -		if (ret)
> -			goto release_tun_meta;
> +		if (!((exists ? (*a)->tcfa_flags : act_flags) &
> +		      TCA_ACT_FLAGS_SKIP_SW)) {
> +			ret = dst_cache_init(&metadata->u.tun_info.dst_cache,
> +					     GFP_KERNEL);
> +			if (ret)
> +				goto release_tun_meta;
> +		}
>  #endif

[Severity: Medium]
Does this cover the usual hardware offload setup? In that setup skip_sw
is set on the filter, not on the action:

  tc filter add ... flower skip_sw ... action tunnel_key set ...

For a new action, exists is false, so act_flags is checked. act_flags
comes from tcf_action_init_1() and holds only the per-action user flags
plus the BIND/REPLACE flags:

net/sched/act_api.c:tcf_action_init_1() {
    ...
	err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp,
			userflags.value | flags, extack);
    ...
}

fl_change() passes the filter flags (fl_flags) through
tcf_exts_validate_ex(). tcf_action_init() copies them onto the action
only after ->init() has returned:

net/sched/act_api.c:tcf_action_init() {
    ...
		act = tcf_action_init_1(net, tp, tb[i], est, ops[i - 1],
					&init_res[i - 1], flags, extack);
    ...
				if (skip_sw)
					act->tcfa_flags |= TCA_ACT_FLAGS_SKIP_SW;
    ...
}

By that point tunnel_key_init() has already called dst_cache_init().
tcf_action_exec() always skips the resulting action, but the action
still holds a per-CPU dst_cache.

This matches the baseline behaviour, so nothing breaks. Still, the
subject says "skip dst cache for skip_sw actions". Only actions with an
explicit skip_sw in TCA_ACT_FLAGS get the saving: standalone actions, or
in-filter actions that set their own flags.

Could the filter's skip_sw reach ->init()? One way would be to OR
TCA_ACT_FLAGS_SKIP_SW into the flags given to tcf_action_init_1() when
tc_skip_sw(fl_flags) is set. Otherwise, could the commit message mention
this limit?

The commit message also doesn't say why the change is wanted (presumably
to save per-CPU memory). Could that be added?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009051930.3619697-1-acassen%40corp.free.fr