Re: [PATCH net-next v5 2/2] net: dsa: yt921x: Add ACL support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: yt921x: Add ACL support

Enable filtering of incoming traffics. Note that custom filters are yet
to be utilized, and thus not all flow dissectors are implemented.

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index a652599a4561b..8afeffcc77579 100644
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c

[ ... ]

> @@ -1457,6 +1467,1017 @@ yt921x_dsa_port_setup_tc(struct dsa_switch *ds, int port,

[ ... ]

> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_TCP)) {
> +		struct flow_match_tcp match;
> +
> +		flow_rule_match_tcp(rule, &match);
> +
> +		if (match.mask->flags) {
> +			entry = yt921x_acl_entries_find(entries, &size,
> +							YT921X_ACL_TYPE_MISC);
> +			if (!entry)
> +				goto err;
> +
> +			entry->key[0] |= YT921X_ACL_BINa_MISC_TCP_FLAGS(ntohs(match.key->flags));
> +			entry->mask[0] |= YT921X_ACL_BINa_MISC_TCP_FLAGS(ntohs(match.mask->flags));

[Severity: High]
Does this code silently truncate the 16-bit TCP flags to 8 bits?

The hardware field for TCP flags YT921X_ACL_BINa_MISC_TCP_FLAGS_M is 8 bits
wide, but standard TCP flags in flow_match_tcp are 16 bits. If a user provides
a mask containing bits beyond the lower 8 bits, it seems the FIELD_PREP macro
will silently truncate the upper bits, causing the hardware rule to match and
permit more traffic than intended by the firewall policy.

Should this validate that ntohs(match.mask->flags) & ~0xFF is zero before
applying the rule?

> +		}
> +	}

[ ... ]

> +static int
> +yt921x_acl_rule_ext_parse_flow_action(struct yt921x_acl_rule_ext *ruleext,

[ ... ]

> +		default:
> +fallback:
> +			if (cls->common.skip_sw) {
> +				NL_SET_ERR_MSG_FMT_MOD(extack,
> +						       "Action not supported when skip_sw: %s",
> +						       reason);
> +				return -EOPNOTSUPP;
> +			}
> +			fallthrough;
> +		case FLOW_ACTION_TRAP:
> +			redir_act = FLOW_ACTION_TRAP;
> +
> +			action[2] &= ~YT921X_ACL_ACTc_FWD_REDIR_DPORTS_M &
> +				     ~YT921X_ACL_ACTc_FWD_M;
> +			action[2] |= YT921X_ACL_ACTc_FWD_EN |
> +				     YT921X_ACL_ACTc_FWD_REDIR;

[Severity: High]
Does this configure the hardware to silently drop packets instead of trapping
them to the CPU?

By setting the YT921X_ACL_ACTc_FWD_REDIR action flag and completely clearing
the destination ports mask, directing a packet to a zero port mask instructs
the hardware to silently drop it. This sequence appears identical to
FLOW_ACTION_DROP.

Should the driver use the dedicated YT921X_ACL_ACTc_FWD_TRAP hardware action
instead?

> +			break;
> +		}

[ ... ]

> +static int
> +yt921x_acl_del(struct yt921x_priv *priv, enum tc_setup_type type,
> +	       unsigned long tag)
> +{

[ ... ]

> +	aclblk->rules[binid] = NULL;
> +	res = yt921x_acl_commit(priv, entid, aclrule->mask);
> +	if (res) {
> +		aclblk->rules[binid] = aclrule;
> +		return res;
> +	}

[Severity: Medium]
Can this leak software state and memory for ACL rules if the hardware deletion
fails?

If yt921x_acl_commit() fails, the function restores the pointer and returns
early without freeing the software state (aclrule, aclblk, and meters_map).
Because the TC subsystem ignores errors from hardware .cls_flower_del
callbacks and unconditionally destroys the generic software filter anyway, the
driver will permanently leak its internal tracking structures for this rule.

> +
> +	if (aclrule->action[0] & YT921X_ACL_ACTa_METER_EN)
> +		clear_bit(FIELD_GET(YT921X_ACL_ACTa_METER_ID_M,
> +				    aclrule->action[0]),
> +			  priv->meters_map);
> +	priv->acl_masks[blkid] &= ~aclrule->mask;
> +	devm_kfree(dev, aclrule);

[Severity: Medium]
Does this cause devres list bloat and spinlock contention?

Calling devm_kfree() on every rule deletion requires acquiring the devres_lock
spinlock and performing an O(N) linear search through the device's managed
resource list. Since TC rules are dynamically added and removed, it might be
better to use standard kzalloc/kfree.

> +	if (!priv->acl_masks[blkid]) {
> +		devm_kfree(dev, aclblk);
> +		priv->acl_blks[blkid] = NULL;
> +	}
> +	return 0;
> +}
> +
> +static int
> +yt921x_acl_add(struct yt921x_priv *priv,
> +	       const struct yt921x_acl_rule_ext *ruleext,
> +	       struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	if (use_meter) {
> +		meterid = find_first_zero_bit(priv->meters_map,
> +					      YT921X_METER_NUM);
> +		if (meterid < YT921X_METER_NUM) {
> +			res = yt921x_meter_config(priv, meterid,
> +						  &ruleext->marker);
> +			if (res)
> +				return res;
> +		} else if (ruleext->r.sw_assisted) {
> +			use_meter = false;

[Severity: High]
Does this silently bypass hardware policing when hardware meters are exhausted?

When a rule has a police action but no hardware meters are available, and
software assistance is allowed, setting use_meter = false installs the rule
into hardware without the meter action.

Since the hardware applies the remaining actions (e.g., forwarding) at line
rate without trapping the packet to the CPU, the software TC datapath never
sees the traffic, completely bypassing the requested rate limit.

If a rate limit cannot be offloaded, should the driver either reject the
offload or explicitly trap matching packets to the CPU?

> +		} else {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "No more meters available");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	/* Prepare acl block ctrlblk */
> +	blkid = entid / YT921X_ACL_ENT_PER_BLK;
> +	binid = entid % YT921X_ACL_ENT_PER_BLK;
> +	aclblk = priv->acl_blks[blkid];
> +	if (!aclblk) {
> +		aclblk = devm_kzalloc(dev, sizeof(*aclblk), GFP_KERNEL);

[Severity: Medium]
Is devm_kzalloc() appropriate for allocating dynamic TC offload rules?

The devm_* APIs are designed for device lifecycle management (probe/remove),
not for dynamic, user-driven allocations like network flows. As mentioned
above regarding devm_kfree, this will bloat the devres list and cause
significant spinlock contention.

> +		if (!aclblk)
> +			return -ENOMEM;
> +		priv->acl_blks[blkid] = aclblk;
> +	}
-- 
pw-bot: cr