Re: [PATCH net v2 1/2] net: openvswitch: do not record user features when the change fails

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says "cls_flower, the bridge, the flow
  dissector and OVS hold references to it" about the tc_skb_ext_tc…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but each failed
  OVS_DP_CMD_NEW leaks one tc_skb_ext_tc static key reference, with no…

--

net: openvswitch: do not record user features when the change fails

This patch moves the store to dp->user_features in ovs_dp_change() so it
happens after the per-cpu upcall port ID update. A failed OVS_DP_CMD_SET
therefore no longer records the new features. The tc_skb_ext_tc enable and
disable decisions now use the local user_features variable.

> tc_skb_ext_tc is one static key for the whole host. cls_flower, the
> bridge, the flow dissector and OVS hold references to it, and the key
> stays on while any of them holds one.

[Severity: Low]
Is this list of reference holders accurate? Only cls_flower and
openvswitch call tc_skb_ext_tc_enable() and tc_skb_ext_tc_disable(). In
net/sched/cls_api.c those two wrap static_branch_inc() and
static_branch_dec().

The bridge and the flow dissector only read the key through
tc_skb_ext_tc_enabled():

net/core/flow_dissector.c:
	if (tc_skb_ext_tc_enabled()) {

net/bridge/br_private.h:
	if (!tc_skb_ext_tc_enabled())

The description of what happens to these readers is still correct. Could
the sentence say that cls_flower and OVS hold references, and that the
bridge and the flow dissector only depend on the key?

> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index b2a0b09fd4a56..00043aef2d030 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c

[ ... ]

> @@ -1812,10 +1810,12 @@ static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
>  			return err;
>  	}
>  
> -	if ((dp->user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
> +	dp->user_features = user_features;
> +
> +	if ((user_features & OVS_DP_F_TC_RECIRC_SHARING) &&
>  	    !(old_features & OVS_DP_F_TC_RECIRC_SHARING))
>  		tc_skb_ext_tc_enable();

[Severity: High]
This is a pre-existing issue that this patch did not introduce. Can every
failed OVS_DP_CMD_NEW leak one tc_skb_ext_tc reference?

In ovs_dp_cmd_new() the dp is newly allocated, so old_features is 0. A
request that carries OVS_DP_F_TC_RECIRC_SHARING always gets to
tc_skb_ext_tc_enable() here.

If new_vport() then fails, for example with -EEXIST because the name is
already taken, the error path frees dp without a matching disable:

net/openvswitch/datapath.c:ovs_dp_cmd_new() {
    ...
	err = ovs_dp_change(dp, a);
	if (err)
		goto err_unlock_and_destroy_meters;

	vport = new_vport(&parms);
	if (IS_ERR(vport)) {
	...
		goto err_destroy_portids;
	}
    ...
err_destroy_portids:
	kfree(rcu_dereference_raw(dp->upcall_portids));
    ...
err_destroy_dp:
	kfree(dp);
    ...
}

The only disable at teardown is in __dp_destroy(), and this path never
gets there:

	if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
		tc_skb_ext_tc_disable();

ovs_dp_reset_user_features() has the same gap. It sets
dp->user_features = 0 on an existing datapath without a disable. After
that, __dp_destroy() will not drop the reference, and a later SET that
turns the bit back on takes another one.

The commit message says these paths "still leave the key one enable
ahead". OVS_DP_CMD_NEW only needs CAP_NET_ADMIN in a user namespace that
owns the netns. Can't the sequence be repeated, adding one enable for
each failure? For example, create datapath X, then keep sending NEW for
X with TC_RECIRC_SHARING.

That would leave the host-wide key stuck on, and cls_flower and OVS
could no longer turn it off. When the count reaches INT_MAX,
static_key_fast_inc_not_disabled() refuses the increment. The
WARN_ON_ONCE() in static_key_slow_inc_cpuslocked() then fires, and that
panics hosts running with panic_on_warn.

The next patch in the series only changes selftests, so this is still
there after the series. Would it make sense to add a
tc_skb_ext_tc_disable() to the ovs_dp_cmd_new() error path and to
ovs_dp_reset_user_features()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108123424.2825403-1-houminxi%40gmail.com