Re: [PATCH net v1 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The commit message says "A failed request then leaves both the
  features and the key alone".
- [Low] The commit message understates who can trigger the fixed bug and
  what happens.

--

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

This patch moves the store of the new features into dp->user_features in
ovs_dp_change() so it happens after ovs_dp_set_upcall_portids() succeeds.
The enable and disable decisions for the tc skb extension static key now
use the local user_features variable. A failed request should no longer
leave the recorded features and the key out of step.

> That key is a
> reference count shared with cls_flower, so the count ends up one short
> and reaches zero while other users still hold a reference.

[Severity: Low]
Could the commit message say more about who can trigger this and what
happens?

OVS_DP_CMD_SET and OVS_DP_CMD_NEW use GENL_UNS_ADMIN_PERM, and
dp_datapath_genl_family sets netnsok, so CAP_NET_ADMIN in a user namespace
is enough. The ops use GENL_DONT_VALIDATE_STRICT and datapath_policy has
no entry for OVS_DP_ATTR_PER_CPU_PIDS. An empty attribute therefore gets
through validation and reaches the -EINVAL in ovs_dp_set_upcall_portids().

tc_skb_ext_tc is a single global key in net/sched/cls_api.c, read by tc,
the bridge, the flow dissector and OVS. One crafted SET from a container
can change skb-ext behaviour for the whole host.

When OVS is the only user of the key, the pre-patch sequence is: a failed
SET records the bit, then a later SET clears it. That calls
tc_skb_ext_tc_disable() with the count at zero, which hits the WARN in
static_key_dec_not_one():

kernel/jump_label.c:static_key_dec_not_one() {
    ...
	if (WARN_ON_ONCE(v == 0))
		return true;
    ...
}

When cls_flower does hold a reference, its own later disable hits the same
WARN. With panic_on_warn set, the system panics.

Would it be worth mentioning the WARN and the user namespace reachability
in the changelog, so stable and security triage can weigh the fix?

> 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: Medium]
The commit message says "A failed request then leaves both the features
and the key alone". Is that true for a failed OVS_DP_CMD_NEW?

In ovs_dp_cmd_new(), ovs_dp_change() runs on a fresh dp where
old_features is 0. A request that carries OVS_DP_F_TC_RECIRC_SHARING
therefore takes the tc_skb_ext_tc_enable() branch above.

new_vport() can then fail. Examples are -EEXIST when a netdev with that
name already exists, -ENOMEM, or an ifindex conflict:

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_unlock_and_destroy_meters:
	ovs_unlock();
	ovs_meters_exit(dp);
    ...
err_destroy_dp:
	kfree(dp);
    ...
}

That unwind never calls tc_skb_ext_tc_disable(). The dp was never added
to ovs_net->dps, so __dp_destroy() does not run for it either.

Does each such request leak one reference on the global tc_skb_ext_tc
key? That would leave the key "enabled for good", which the commit
message lists as part of the bug.

The -EEXIST branch has a second path. When genlhdr->version is below
OVS_DP_VER_FEATURES, it calls ovs_dp_reset_user_features() on the
existing datapath:

net/openvswitch/datapath.c:ovs_dp_reset_user_features() {
    ...
	dp->user_features = 0;
}

This clears OVS_DP_F_TC_RECIRC_SHARING without calling
tc_skb_ext_tc_disable(). __dp_destroy() decides from dp->user_features:

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

so that reference is never dropped. A later SET that sets the bit again
takes another one.

The commit message does name both paths and leaves them out of scope. Both
end in the same state this patch fixes in ovs_dp_change(): the bit in
dp->user_features no longer matches exactly one held key reference. Both
can be reached with CAP_NET_ADMIN in a user namespace, and the effect
applies to the whole host.

Two possible fixes:

- In the NEW unwind, call tc_skb_ext_tc_disable() when dp->user_features
  has the bit, or delay the enable until the request succeeds.

- In ovs_dp_reset_user_features(), call tc_skb_ext_tc_disable() when it
  clears the bit.

Since the patch has Cc: stable, could the closing sentence be narrowed?
Otherwise backporters may read it as meaning failed requests now keep the
key balanced in all cases.

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