Re: [PATCH 14/20] net: dsa: xilinx: program MAC frame filter and per-port nibbles

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Use-After-Free and API violation when broadcasting NETDEV_CHANGEADDR on unregistered user ports.
- [High] Silent mutation of MAC address during NETDEV_REGISTER causes state divergence across subsystems.
- [High] Bypassing DSA MAC management leaks resources and drops packets after a conduit MAC change.
- [Medium] Missing collision check during initial probe allows duplicate MAC nibbles to break switch routing.
--

--- Patch [14]: [PATCH 14/20] net: dsa: xilinx: program MAC frame filter and per-port nibbles ---
commit a960829c1e4fd1b836fd23be3fc9bec389a6b123
Author: Nagadheeraj Rottela <[email protected]>

net: dsa: xilinx: program MAC frame filter and per-port nibbles

This commit programs the switch fabric MAC frame filter using a 44-bit
prefix from the conduit interface and configures a unique 4-bit nibble for
each port. It adds a netdev notifier to track conduit and user port MAC
changes to keep the hardware synchronized.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 86aba2e8b6e94..9826f006b0789 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> @@ -287,6 +357,112 @@ static int xlnx_tsn_mdio_register_all(struct xlnx_tsn *sw)
>  	return ret;
>  }
>  
> +/* Build a per-port MAC from the shared prefix. */
> +static void xlnx_tsn_synth_port_mac(struct xlnx_tsn *sw, int port,
> +				    u8 *out)

[ ... ]

> +static int xlnx_tsn_handle_user_register(struct xlnx_tsn *sw,
> +					 struct net_device *dev, int port)
> +{
> +	u8 want[ETH_ALEN];
> +	u8 nibble;
> +
> +	if (!xlnx_tsn_prefix_matches(sw, dev->dev_addr)) {
> +		xlnx_tsn_synth_port_mac(sw, port, want);
> +		dev_warn(sw->dev,
> +			 "port %d: MAC %pM does not match conduit prefix; overriding to %pM\n",
> +			 port, dev->dev_addr, want);
> +		dev_addr_mod(dev, 0, want, ETH_ALEN);

[Severity: High]
Does modifying the MAC address directly in the registration notification
chain leave earlier subsystems out of sync?

When xlnx_tsn_handle_user_register() calls dev_addr_mod() during
NETDEV_REGISTER, subsystems like IPv6 that have already run earlier in the
notification chain will observe the old MAC inherited from the conduit,
and no subsequent NETDEV_CHANGEADDR is emitted to correct their state.

> +		nibble = want[5] & TSN_SW_MAC_NIBBLE_WILDCARD;
> +	} else if (ether_addr_equal(dev->dev_addr, sw->conduit->dev_addr)) {
> +		/* Either DSA inherited the conduit MAC, or DT gave port@N
> +		 * the same address explicitly. Either way, assign a unique
> +		 * per-port nibble.
> +		 */
> +		xlnx_tsn_synth_port_mac(sw, port, want);
> +		dev_addr_mod(dev, 0, want, ETH_ALEN);
> +		nibble = want[5] & TSN_SW_MAC_NIBBLE_WILDCARD;
> +	} else {
> +		nibble = dev->dev_addr[5] & TSN_SW_MAC_NIBBLE_WILDCARD;
> +	}
> +
> +	return xlnx_tsn_set_port_mac_nibble(sw, port, nibble);

[Severity: Medium]
Is it possible for the switch hardware to be configured with duplicate port
identifiers here?

Unlike the runtime callback, this initial registration path unconditionally
extracts and programs the nibble without verifying if it collides with the
CPU port or another user port. This could corrupt switch routing if the
device tree configures identical MAC addresses during probe.

> +}
> +
> +static void xlnx_tsn_handle_conduit_changeaddr(struct xlnx_tsn *sw)
> +{
> +	struct dsa_port *dp;
> +
> +	xlnx_tsn_derive_prefix(sw);
> +	xlnx_tsn_program_frame_filter(sw);
> +	xlnx_tsn_set_port_mac_nibble(sw, XLNX_TSN_CPU_PORT,
> +				     sw->conduit->dev_addr[5]);
> +
> +	dsa_switch_for_each_user_port(dp, &sw->ds) {
> +		u8 want[ETH_ALEN];
> +
> +		if (!dp->user)
> +			continue;
> +
> +		xlnx_tsn_synth_port_mac(sw, dp->index, want);
> +		dev_addr_mod(dp->user, 0, want, ETH_ALEN);

[Severity: High]
Does bypassing the DSA core ndo_set_mac_address hook leak resources and
cause packets to be dropped?

By using dev_addr_mod() here, the DSA core dsa_user_set_mac_address() is
never invoked. This means dsa_user_host_uc_install() and
dsa_user_host_uc_uninstall() are not called to synchronize the hardware
unicast filters with the new MAC address, leaving the old user MAC address
leaked in the conduit's filter list.

> +		call_netdevice_notifiers(NETDEV_CHANGEADDR, dp->user);

[Severity: High]
Can this lead to a use-after-free or API violation?

If the conduit MAC address is changed concurrently with a failure in
dsa_user_create(), dp->user might be cleared without holding rtnl_lock,
allowing this code to dereference freed memory.

Also, this unconditionally broadcasts NETDEV_CHANGEADDR for a device that
may not have completed register_netdevice() yet (where reg_state ==
NETREG_UNINITIALIZED), which could confuse listeners.

> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14