Re: [PATCH net-next v2 4/5] net: dsa: motorcomm: Enable ACL on demand

[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] Memory leak on an error path in yt921x_acl_add()
  (drivers/net/dsa/motorcomm/tc.c).
- [Low] The hardware ACL port-enable register can disagree with the
  software acl_cnt bookkeeping after errors.

--

net: dsa: motorcomm: Enable ACL on demand

This patch stops yt921x_chip_setup_acl() from enabling ACL on all ports.
It adds a per-port acl_cnt counter instead. yt921x_acl_add() sets a
port's YT921X_ACL_PORT bit when the port gets its first rule, and
yt921x_acl_del() refreshes the register once the last rule for that
port is removed.

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> index 0e5f19a39d0cc..95ed0f245b1fa 100644
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c

[ ... ]

> @@ -1312,6 +1347,19 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  	else
>  		aclrule->action[0] &= ~YT921X_ACL_ACTa_METER_EN;
>  
> +	/* Activate ACL if needed */
> +	ctrl = 0;
> +	m = FIELD_GET(YT921X_ACL_KEYb_SPORTS_M, ruleext->r.entries[0].key[1]);
> +	for_each_set_bit(o, &m, YT921X_PORT_NUM)
> +		if (!priv->ports[o].acl_cnt)
> +			ctrl |= YT921X_ACL_PORT_PORTn(o);
> +	if (ctrl) {
> +		ctrl |= yt921x_acl_activated_ports(priv);
> +		res = yt921x_reg_write(priv, YT921X_ACL_PORT, ctrl);
> +		if (res)
> +			return res;

[Severity: Medium]
Does this early return leak aclrule?

At this point aclrule has already been allocated by kvmemdup(), but it
hasn't been stored in aclblk->rules[binid] yet:

	aclrule = kvmemdup(&ruleext->r,
			   offsetof(struct yt921x_acl_rule, entries[entscnt]),
			   GFP_KERNEL);

The teardown in chip.c only frees rules it can reach through
priv->acl_blks[]->rules[], so nothing would ever free this one.

The return also skips the err: label:

err:
	if (!priv->acl_masks[blkid]) {
		kvfree(aclblk);
		priv->acl_blks[blkid] = NULL;
	}

If aclblk was freshly allocated for this rule, it stays in
priv->acl_blks[blkid] even though the block is empty. That isn't a
permanent leak, since a later add can reuse it and remove frees it.

This happens when a source port has acl_cnt == 0 and the YT921X_ACL_PORT
write fails. Should this path do kvfree(aclrule) and then goto err,
like the yt921x_acl_commit() failure path does?

> +	}
> +
>  	/* Write rules */
>  	aclblk->rules[binid] = aclrule;
>  	res = yt921x_acl_commit(priv, entid, aclrule->mask);

[Severity: Low]
Can YT921X_ACL_PORT end up out of sync with acl_cnt here?

The port bits are set in hardware before yt921x_acl_commit() runs. If
any register write inside yt921x_acl_commit() fails, the existing error
branch only rolls back software state:

	if (res) {
		aclblk->rules[binid] = NULL;
		kvfree(aclrule);
		goto err;
	}

acl_cnt++ only runs after a successful commit. So the port stays
ACL-enabled in hardware while acl_cnt for it is still 0.

yt921x_acl_del() has the same problem in reverse. acl_cnt is decremented
to 0 first, and then the register is refreshed:

	if (refresh_en) {
		ctrl = yt921x_acl_activated_ports(priv);
		res = yt921x_reg_write(priv, YT921X_ACL_PORT, ctrl);
		if (res)
			ret = res;
	}

If that write fails, the bit stays set while software counts no rules
on the port.

In both cases the stale bit is only cleared when a later add or del
rewrites the whole register. Unmatched traffic is still permitted
because YT921X_ACL_PERMIT_UNMATCH is set for all ports, but the port
keeps doing ACL lookups when ACL isn't "actually used" on it.

Should the yt921x_acl_add() commit failure path write
yt921x_acl_activated_ports(priv) back to YT921X_ACL_PORT?

[ ... ]

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