Re: [PATCH net] net/sched: release old template when replacing a chain

[netdev-bot+sashiko@kernel.org]

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

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

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

- [High] The patch adds a use-after-free race on chain->tmplt_priv.
- [Medium] The patch treats a symptom.
- [Medium] tc_chain_tmplt_add() creates and offloads the new template
  (fl_tmplt_create() -> fl_hw_create_tmplt() sends…
- [Low] The commit message reads like a bug-report email rather than a
  kernel changelog.

--

net/sched: release old template when replacing a chain

With this patch, tc_chain_tmplt_add() destroys any template already on
the chain once the new template has been created. It targets a chain that
a goto-chain action keeps alive after RTM_DELCHAIN and that RTM_NEWCHAIN
then recreates. In that case the old template and its module reference
used to leak.

> Hello,

[ ... ]

> HEAD commit:    c03c1a14507dfaf8b856c2608f9ac216021fc222
> Git tree:       30fd2ef82912c26cf3a58ad0ba6afc375fa851ac
> Source version: Linux 7.3.0
> Compiler:       gcc 11.4.0, GNU ld 2.38
>
> The console output, kernel config, reproducer, namespace launcher, and
> patched-kernel validation logs are available here:

[ ... ]

[Severity: Low]
This isn't a bug, but the changelog reads like a bug report email. It has
the "Hello," greeting, reproducer instructions, the gist URLs, QEMU test
logistics, the reporter's tree SHAs and compiler versions. It also has the
note that the positive PoC reports a failed threshold on the patched
kernel.

Could this material go below the --- line or into a cover letter, so that
the changelog only covers the problem, its impact and the fix?

> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index bd0d635fe3fed..59f96e899c214 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3143,6 +3143,8 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net,
>  		module_put(ops->owner);
>  		return PTR_ERR(tmplt_priv);
>  	}
> +	/* An action-held chain can retain a template after RTM_DELCHAIN. */

[Severity: Medium]
Should the template survive RTM_DELCHAIN at all? Right now
__tcf_chain_put() only releases it when the chain is finally freed:

net/sched/cls_api.c:__tcf_chain_put() {
    ...
	if (refcnt == 0) {
		tc_chain_tmplt_del(tmplt_ops, tmplt_priv);
		tcf_chain_destroy(chain, free_block);
	}
}

While action_refcnt is non-zero, the delete notification is still sent
through tc_chain_notify_delete(). The chain is then hidden from dumps,
RTM_GETCHAIN and RTM_DELCHAIN, but its template stays active.

This has a few side effects that the patch doesn't address.

After chain N is deleted, tc_new_tfilter() still gets the same chain
through tcf_chain_get(block, N, true). It rejects other filter kinds with
"Chain template is set to a different filter kind":

	if (chain->tmplt_ops && chain->tmplt_ops != tp->ops) {

An RTM_NEWCHAIN without TCA_KIND on that chain takes this early return in
tc_chain_tmplt_add(), so the recreated chain silently inherits the old
template:

	/* If kind is not set, user did not specify template. */
	if (!tca[TCA_KIND])
		return 0;

The old template's hardware offload also stays installed after the
delete.

The new comment describes this retention as expected behaviour. Would it
be better to release the template when the chain becomes action-only,
with suitable synchronization against readers? That would fix the leak
and these cases, and this hunk would not be needed.

> +	tc_chain_tmplt_del(chain->tmplt_ops, chain->tmplt_priv);

[Severity: High]
Can this free the old struct fl_flow_tmplt while an unlocked filter add
is still reading it?

tc_ctl_chain() drops block->lock before it calls tc_chain_tmplt_add(),
so only rtnl is held at this point. RTM_NEWTFILTER is
RTNL_FLAG_DOIT_UNLOCKED. For flower on ingress/clsact or on a shared
block, tc_new_tfilter() skips rtnl_lock() altogether. fl_change() then
reads tp->chain->tmplt_priv without any lock.

CPU0 (RTM_NEWCHAIN, holds only rtnl)
tc_ctl_chain()
    chain->explicitly_created = true;
    mutex_unlock(&block->lock);

CPU1 (flower RTM_NEWTFILTER on chain N, no rtnl)
tc_new_tfilter()
    tcf_chain_get(block, N, true);
    passes the chain->tmplt_ops check
    fl_change()
        loads tp->chain->tmplt_priv (the old template)

CPU0
tc_chain_tmplt_add()
    ops->tmplt_create()
    tc_chain_tmplt_del()
        fl_tmplt_destroy()
            fl_hw_destroy_tmplt(tmplt->chain, tmplt);
            kfree(tmplt);

CPU1
fl_mask_fits_tmplt(tp->chain->tmplt_priv, mask)
    reads tmplt->mask from the freed object

The free also happens before the new pointers are stored. A reader that
loads chain->tmplt_priv between the kfree() and this store gets a pointer
that is already dangling:

	chain->tmplt_priv = tmplt_priv;

Before this patch, the only template free was in __tcf_chain_put() at
refcnt == 0, when nothing else could hold the chain. The commit message
notes that a user namespace is enough for CAP_NET_ADMIN, and running
DELCHAIN/NEWCHAIN cycles next to concurrent flower filter adds would
reach this window.

[Severity: Medium]
With this order, fl_hw_create_tmplt() sends FLOW_CLS_TMPLT_CREATE for the
new template before fl_hw_destroy_tmplt() sends FLOW_CLS_TMPLT_DESTROY for
the old one. Both use the same chain_index. Can that leave offload drivers
in the wrong state?

The in-tree drivers seem to assume one template per chain index, and they
match DESTROY by chain_index only. In sparx5, create records the current
port keyset as orig:

drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c:sparx5_tc_flower_template_create() {
    ...
	sparx5_vcap_set_port_keyset(ndev, admin, fco->common.chain_index,
				    state.l3_proto,
				    ftp->keyset,
				    &kslist);

	if (kslist.cnt > 0)
		ftp->orig = kslist.keysets[0];
    ...
}

Here orig is the old template's keyset. Destroy then restores orig from
the first entry with a matching cid, which is the old template's entry:

drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c:sparx5_tc_flower_template_destroy() {
    ...
	list_for_each_entry_safe(ftp, tmp, &port->tc_templates, list) {
		if (ftp->cid != fco->common.chain_index)
			continue;

		sparx5_vcap_set_port_keyset(ndev, admin,
					    fco->common.chain_index,
					    ftp->l3_proto, ftp->orig,
					    NULL);
    ...
}

So the port runs the keyset from before any template while the new
template is active. Once the new template is destroyed, the port goes
back to the old template's keyset.

In mlxsw, mlxsw_sp_flower_tmplt_create() reuses the existing ruleset for
the chain index through mlxsw_sp_acl_ruleset_get(), so the new template's
elusage is ignored. In prestera, prestera_acl_ruleset_keymask_set()
overwrites ruleset->keymask with kmemdup() without freeing the previous
one, so each cycle leaks one keymask.

The mlxsw and prestera behaviour already existed before this patch, but
create-first keeps it, while destroy-then-create would let the ruleset be
released and rebuilt.

The commit message says create-first keeps the old template intact if
creation fails. Would destroying the old template first give drivers a
consistent sequence?

>  	chain->tmplt_ops = ops;
>  	chain->tmplt_priv = tmplt_priv;
>  	return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109121448.2589374-1-shawnq1%40163.com