Re: [PATCH] mlxsw: spectrum_flower: Fix port range register leak

[netdev-bot+sashiko@kernel.org]

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

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

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] Incomplete fix: the port-range register leak the commit message
  claims to close remains fully reachable.

--

mlxsw: spectrum_flower: Fix port range register leak

Adds an unwind to mlxsw_sp_flower_parse_ports_range() so that the source
port range register is released when acquiring the destination port range
register fails.  The commit message motivates this by callers that pass a
stack allocated rule info and never release it.

> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
> index 353fd9ca89a68..4cad6c46b07e9 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
> @@ -486,8 +486,14 @@ mlxsw_sp_flower_parse_ports_range(struct mlxsw_sp *mlxsw_sp,
>  
>  		err = mlxsw_sp_port_range_reg_get(mlxsw_sp, &range,
>  						  f->common.extack, &prr_index);
> -		if (err)
> +		if (err) {
> +			if (rulei->src_port_range_reg_valid) {
> +				mlxsw_sp_port_range_reg_put(mlxsw_sp,
> +							    rulei->src_port_range_reg_index);
> +				rulei->src_port_range_reg_valid = false;
> +			}
>  			return err;
> +		}
>  
>  		rulei->dst_port_range_reg_index = prr_index;
>  		rulei->dst_port_range_reg_valid = true;

[Severity: High]

Does this hunk actually close the leak the changelog describes?

mlxsw_sp_flower_parse() has two callers.  The first is
mlxsw_sp_flower_replace(), which uses the heap rulei owned by the rule and
routes every error through mlxsw_sp_acl_rule_destroy() ->
mlxsw_sp_acl_rulei_destroy(), which already drops both registers:

drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c:mlxsw_sp_acl_rulei_destroy() {
	if (rulei->src_port_range_reg_valid)
		mlxsw_sp_port_range_reg_put(mlxsw_sp,
					    rulei->src_port_range_reg_index);
	if (rulei->dst_port_range_reg_valid)
		mlxsw_sp_port_range_reg_put(mlxsw_sp,
					    rulei->dst_port_range_reg_index);
	kfree(rulei);
}

So on the replace path the new put looks redundant.

The second caller is the stack rulei one named in the changelog,
mlxsw_sp_flower_tmplt_create():

	struct mlxsw_sp_acl_rule_info rulei;
	...
	memset(&rulei, 0, sizeof(rulei));
	err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f);
	if (err)
		return err;
	ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block,
					   f->common.chain_index,
					   MLXSW_SP_ACL_PROFILE_FLOWER,
					   &rulei.values.elusage);

	/* keep the reference to the ruleset */
	return PTR_ERR_OR_ZERO(ruleset);

This never calls mlxsw_sp_acl_rulei_destroy() (it cannot, that function ends
in kfree(rulei)), and mlxsw_sp_flower_tmplt_destroy() only drops ruleset
references.  Isn't every port range register recorded in that stack rulei
still leaked?

The new unwind only covers the narrow window "source acquired, destination
acquisition failed".  For the template caller the reference is still dropped
on the floor in these cases:

  - the success path, where one or two registers stay pinned per template
    (this is the common case)
  - later failures inside mlxsw_sp_flower_parse() after
    mlxsw_sp_flower_parse_ports_range() already succeeded, namely
    mlxsw_sp_flower_parse_tcp() rejecting reserved flags,
    mlxsw_sp_flower_parse_ip() rejecting a non-IP n_proto, and
    mlxsw_sp_flower_parse_actions() failing:

	err = mlxsw_sp_flower_parse_ports_range(mlxsw_sp, rulei, f, ip_proto);
	if (err)
		return err;

	err = mlxsw_sp_flower_parse_tcp(mlxsw_sp, rulei, f, ip_proto);
	if (err)
		return err;

	err = mlxsw_sp_flower_parse_ip(mlxsw_sp, rulei, f, n_proto_key & n_proto_mask);
	if (err)
		return err;

	return mlxsw_sp_flower_parse_actions(mlxsw_sp, block, rulei,
					     &f->rule->action,
					     f->common.extack);

  - mlxsw_sp_acl_ruleset_get() returning an error in
    mlxsw_sp_flower_tmplt_create()

Is the template key really non-zero here?  fl_tmplt_create() in
net/sched/cls_flower.c fills tmplt->dummy_key and tmplt->mask from the user
attributes via fl_set_key(), fl_set_key_port_range() sets
mask->tp_range.tp_min.src/dst to all ones for TCP/UDP/SCTP, and
fl_hw_create_tmplt() hands that dissector, key and mask to the driver.
FLOW_DISSECTOR_KEY_PORTS_RANGE is in the accepted key mask in
mlxsw_sp_flower_parse(), so mlxsw_sp_port_range_reg_get() takes a real
reference for a template such as:

	tc chain add dev swp1 ingress protocol ip \
		flower ip_proto tcp src_port 100-200

The pool is small and fixed.  mlxsw_sp_port_range_init() sets
pr_core->prr_ids.max from MLXSW_CORE_RES_GET(core, ACL_MAX_L4_PORT_RANGE)
and warns above 16 entries:

	max = MLXSW_CORE_RES_GET(core, ACL_MAX_L4_PORT_RANGE);

	/* Each port range register is represented using a single bit in the
	 * two bytes "l4_port_range" ACL key element.
	 */
	WARN_ON(max > BITS_PER_BYTE * sizeof(u16));

Once it is exhausted, mlxsw_sp_port_range_reg_create() fails:

	err = xa_alloc(&pr_core->prr_xa, &prr->index, prr, pr_core->prr_ids,
		       GFP_KERNEL);
	if (err) {
		if (err == -EBUSY)
			NL_SET_ERR_MSG_MOD(extack, "Exceeded number of port range registers");

Can repeated template add/delete with distinct port ranges therefore pin all
registers for the lifetime of the device, keep devlink prr_count inflated,
and make mlxsw_sp_port_range_fini()'s WARN_ON(!xa_empty(&pr_core->prr_xa))
fire on teardown?

Would it be better to drop this hunk and instead release the rulei's port
range registers in mlxsw_sp_flower_tmplt_create(), for example through a
non-freeing rulei cleanup helper that mirrors mlxsw_sp_acl_rulei_destroy(),
or to skip register acquisition altogether when parsing a template?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917113236.2149095-1-vulab%40iscas.ac.cn