Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: check the port when deleting a policy rule

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

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

- [Medium] Missing `Fixes:` tag.

Pre-existing issues:
- [Medium] In the ETHTOOL_SRXCLSRLDEL path of mv88e6xxx_set_rxnfc() the
  policy object is unlinked from chip->policies (idr_remove(),…

--

net: dsa: mv88e6xxx: check the port when deleting a policy rule

This changes the ETHTOOL_SRXCLSRLDEL path in mv88e6xxx_set_rxnfc() to look
the policy up with idr_find() and only remove it when policy->port matches
the port the request arrived on, so one port can no longer be used as a
handle to delete another port's rule.

[Severity: Medium]
Should this carry a Fixes: tag?

git blame of the pre-image of this hunk attributes the whole delete
sequence to da7dc87553046 ("net: dsa: mv88e6xxx: add RXNFC support"),
which went in for v5.4, so the broken cross-port delete exists in every
stable tree since then.  Without the trailer the stable tooling won't pick
this up:

	Fixes: da7dc87553046 ("net: dsa: mv88e6xxx: add RXNFC support")

 			policy->action = MV88E6XXX_POLICY_ACTION_NORMAL;
 			err = mv88e6xxx_policy_apply(chip, port, policy);
 			devm_kfree(chip->dev, policy);

[Severity: Medium]
This is a pre-existing issue and not something this patch introduces, but
while the delete path is being touched: should the idr_remove() and
devm_kfree() be conditional on mv88e6xxx_policy_apply() succeeding?

mv88e6xxx_policy_apply() can fail on a register/bus error, both in the ATU
rewrite and in the policy register write:

drivers/net/dsa/mv88e6xxx/chip.c:mv88e6xxx_policy_apply() {
	...
		err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
						   state);
		if (err)
			return err;
	...
	return chip->info->ops->port_set_policy(chip, port, mapping, action);
}

In that case the switch still has the ATU entry in
MV88E6XXX_G1_ATU_DATA_STATE_{UC,MC}_STATIC_POLICY and/or still has the
port's policy register mapping DA/SA to DISCARD, but the software object
and its IDR location are already gone.

Does that leave the rule unaddressable from userspace?  ETHTOOL_GRXCLSRLALL
no longer lists it, a retried ETHTOOL_SRXCLSRLDEL now returns -ENOENT
because idr_find() returns NULL, and the still-programmed mapping is
invisible to the in-use scan at the end of mv88e6xxx_policy_apply():

	if (action == MV88E6XXX_POLICY_ACTION_NORMAL)
		idr_for_each_entry(&chip->policies, policy, id)
			if (policy->port == port &&
			    policy->mapping == mapping &&
			    policy->action != action)
				return 0;

The freed id can also be handed out again by a later ETHTOOL_SRXCLSRLINS,
so a new location would describe hardware programmed for a different MAC.

Doing the idr_remove()/devm_kfree() only after mv88e6xxx_policy_apply()
returns 0 would avoid this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918185326.3940857-1-kuba%40kernel.org