Re: [PATCH net v2 1/3] net/sched: cls_u32: feed u32_replace_hw_knode() with correct set of flags

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net/sched: cls_u32: feed u32_replace_hw_knode() with correct set of flags

The patch changes the two u32_replace_hw_knode() calls in u32_change() to
pass userflags (the TCA_CLS_FLAGS_* set parsed from TCA_U32_FLAGS) instead
of flags (the TCA_ACT_FLAGS_* set passed in as a function parameter), so
that tc_skip_sw() inside the helper looks at the bits userspace actually
set.

Two questions below about what becomes reachable once those flags are
correct.

> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index c297d7dbcf916..1374da97836eb 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -935,7 +935,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  
>  		u32_bind_filter(tp, new, base, tb);
>  
> -		err = u32_replace_hw_knode(tp, new, flags, extack);
> +		err = u32_replace_hw_knode(tp, new, userflags, extack);
>  		if (err) {
>  			u32_unbind_filter(tp, new, tb);
>  

[Severity: High]

Does this make the error path of the replace branch reachable from
userspace for the first time, and can it leave the old knode installed
while its hardware rule is gone?

Before the change, flags only carried TCA_ACT_FLAGS_* bits, so
tc_skip_sw(flags) in u32_replace_hw_knode() was false, tc_setup_cb_add()
ran with err_stop == false and could not return an error, and the
skip_sw && !IN_HW -EINVAL check in the helper was dead code.

With userflags, skip_sw is true for a skip_sw filter, tc_setup_cb_add()
propagates the first driver error, and u32_replace_hw_knode() takes:

net/sched/cls_u32.c:u32_replace_hw_knode() {
	err = tc_setup_cb_add(block, tp, TC_SETUP_CLSU32, &cls_u32, skip_sw,
			      &n->flags, &n->in_hw_count, true);
	if (err) {
		u32_remove_hw_knode(tp, n, NULL);
		return err;
	}
}

Here n is the replacement node, and u32_init_knode() copied the handle
from the old one:

	new->handle = n->handle;

so the TC_CLSU32_DELETE_KNODE emitted by u32_remove_hw_knode() names the
handle of the knode that is still linked in ht->ht[]. u32_change() then
only unbinds and frees new and returns the error, so the old knode stays
in the software list with TCA_CLS_FLAGS_SKIP_SW set, and u32_classify()
skips SKIP_SW knodes. Would that filter then match neither in software
nor in hardware, while userspace was told the command failed?

There also looks like accounting fallout on the same path.
u32_init_knode() copies the flags too:

	new->flags = n->flags;

so new->flags can carry TCA_CLS_FLAGS_IN_HW while new->in_hw_count is
still 0. The teardown then reaches
tc_setup_cb_destroy() -> tc_cls_offload_cnt_reset(), which decrements
purely on the inherited bit:

net/sched/cls_api.c:tc_cls_offload_cnt_reset() {
	spin_lock(&tp->lock);
	tcf_block_offload_dec(block, flags);
	*cnt = 0;
	spin_unlock(&tp->lock);
}

Can this decrement block->offloadcnt for a node that never incremented
it, and then have the old knode (whose flags still carry IN_HW)
decrement it a second time when it is eventually deleted, leaving
tcf_block_offload_in_use() permanently true and later binds of
non-offload-capable devices to the block failing with -EOPNOTSUPP?

For comparison, cls_flower builds fnew->flags only from the netlink
attributes rather than inheriting them from fold, so it does not have
the accounting half of this.

The later patches in this range only adjust ht_down refcounting, so this
error path is unchanged by them.

> @@ -1168,7 +1168,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  		struct tc_u_knode __rcu **ins;
>  		struct tc_u_knode *pins;
>  
> -		err = u32_replace_hw_knode(tp, n, flags, extack);
> +		err = u32_replace_hw_knode(tp, n, userflags, extack);
>  		if (err)
>  			goto errunbind;
>  

[Severity: High]

This isn't a bug introduced by this patch, but now that the real
TCA_CLS_FLAGS_* set reaches these call sites, the TCA_CLS_FLAGS_SKIP_HW
bit is visible here and still nothing acts on it. Should these calls be
gated on tc_skip_hw()?

Neither helper looks at it. u32_replace_hw_knode() and
u32_replace_hw_hnode() only derive:

	bool skip_sw = tc_skip_sw(flags);

and the core does not filter on the classifier's behalf either.
tc_setup_cb_add() only checks the block state before dispatching:

net/sched/cls_api.c:tc_setup_cb_add() {
	/* Make sure all netdevs sharing this block are offload-capable. */
	if (block->nooffloaddevcnt && err_stop) {
		ok_count = -EOPNOTSUPP;
		goto err_unlock;
	}

	ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
}

The peer classifiers gate the call themselves, for example cls_flower:

	if (!tc_skip_hw(fnew->flags))
		err = fl_hw_replace_filter(tp, fnew, rtnl_held, extack);

and cls_matchall:

	if (!tc_skip_hw(new->flags))
		err = mall_replace_hw_filter(tp, new, cookie, extack);

Does a "tc filter add ... u32 skip_hw ..." command therefore still get
pushed to TC_SETUP_CLSU32 drivers and pick up TCA_CLS_FLAGS_IN_HW plus
in_hw_count / block->offloadcnt increments, contradicting the documented
meaning of the flag in include/uapi/linux/pkt_cls.h?

The two consumers of the same flags field also disagree. On block
unbind, tcf_block_playback_offloads() -> u32_reoffload(add=false) does
honour the bit:

net/sched/cls_u32.c:u32_reoffload() {
			if (tc_skip_hw(n->flags))
				continue;
}

so a knode that was offloaded by the add path is skipped here, and its
IN_HW bit, in_hw_count and block->offloadcnt contribution are never
released. Can that leave tcf_block_offload_in_use() true forever and
reject later binds of non-offload-capable devices to that shared block
with -EOPNOTSUPP?

The same missing guard applies to the u32_replace_hw_hnode() call in the
TCA_U32_DIVISOR branch of u32_change(), which is likewise given
userflags with no skip_hw check.
-- 
This is an AI-generated review.