net/sched: release old template when replacing a chain

net/sched/cls_api.c static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, · line 3146

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

net/sched/cls_api.c static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net, · line 3132

[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?