Re: [PATCH net-next 7/9] net: sparx5: add L3 router infrastructure and leg management

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: sparx5: add L3 router infrastructure and leg management

This commit adds L3 router infrastructure and leg management to the
Microchip Sparx5 switch driver. It handles netdevice and inetaddr events
to intercept VLAN uppers of a bridge device, associating them with router
legs (VMIDs) for hardware offloading.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_router.c b/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> new file mode 100644
> index 0000000000000..03923d91fdfb8
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_router.c

[ ... ]

> +static void sparx5_rr_inet6_make_mask_le(int logmask, u8 *mask)
> +{
> +	/* Caller must ensure 0 <= logmask <= 128 */
> +	int rem, byte_prefix = logmask;
> +
> +	rem = do_div(byte_prefix, BITS_PER_BYTE);

[Severity: High]
Will this do_div() call cause a build failure or stack corruption?

The do_div() macro expects a 64-bit unsigned integer for the dividend. Passing
byte_prefix, which is a 32-bit int, causes a compiler error on architectures
like x86. On architectures like ARM, it can overwrite stack memory since the
macro assumes the pointer points to a 64-bit value.

Could byte_prefix be defined as a u64, or could this use a standard division
operation instead?

[ ... ]

> +static bool
> +sparx5_rr_router_leg_addr_list_empty_rcu(struct sparx5_rr_router_leg *leg)
> +{
> +	struct inet6_dev *inet6_dev;
> +	struct in_device *in_dev;
> +
> +	in_dev = __in_dev_get_rcu(leg->dev);
> +	if (in_dev && in_dev->ifa_list)
> +		return false;

[Severity: Medium]
Does this code miss an RCU pointer annotation?

The ifa_list pointer in struct in_device is annotated with __rcu, so accessing
it directly violates RCU rules and can cause sparse warnings.

Could this be updated to use rcu_access_pointer() since it's just doing a
boolean check?

[ ... ]

> +/* Called in atomic context. */
> +static int sparx5_rr_inet6addr_event(struct notifier_block *nb,
> +				     unsigned long event, void *ptr)
> +{
> +	struct inet6_ifaddr *if6 = (struct inet6_ifaddr *)ptr;
> +	struct sparx5_rr_inet6addr_event_work *work;
> +	struct net_device *dev = if6->idev->dev;
> +	struct sparx5_router *router;
> +
> +	if (event != NETDEV_DOWN)
> +		return NOTIFY_DONE;
> +
> +	work = kzalloc_obj(*work, GFP_ATOMIC);
> +	if (!work)
> +		return NOTIFY_BAD;

[Severity: High]
Does returning NOTIFY_BAD during a NETDEV_DOWN event illegally abort the atomic
notification chain?

The inet6addr_chain is an atomic notifier chain, so returning NOTIFY_BAD stops
chain traversal. Because NETDEV_DOWN is a post-action event indicating the
address has already been removed, halting the chain prevents subsequent
notifiers (e.g., routing tables, IPsec) from observing the removal. This
leaves stale state that can cause use-after-free or routing blackholes.

Should this driver log an error and return NOTIFY_DONE or NOTIFY_OK instead
if memory allocation fails?

[ ... ]

> +/* Called with RTNL. */
> +static int sparx5_rr_inetaddr_valid_event(struct notifier_block *nb,
> +					  unsigned long event, void *ptr)
> +{
> +	struct in_validator_info *ivi = (struct in_validator_info *)ptr;
> +	struct net_device *dev = ivi->ivi_dev->dev;
> +	struct sparx5_router *router;
> +
> +	ASSERT_RTNL();
> +
> +	if (event != NETDEV_UP)
> +		return NOTIFY_DONE;
> +
> +	router = container_of(nb, struct sparx5_router, inetaddr_valid_nb);
> +
> +	return sparx5_rr_inetaddr_event_handle(router->sparx5, dev, event);
> +}

[Severity: High]
Does allocating hardware resources in the validator chain cause a leak?

The driver allocates a hardware router leg (VMID) via
sparx5_rr_router_leg_create() and takes a netdev_hold reference during the
NETDEV_UP validation phase.

If a downstream validator rejects the address addition, or if the addition
natively fails, NETDEV_DOWN is never fired. Because there is no rollback hook
handling this failure, the VMID and net_device tracker reference are leaked. A
leaked reference permanently prevents the net_device from being unregistered,
hanging unregister_netdevice() in netdev_wait_allrefs().

[ ... ]

> +/* Called with RTNL. */
> +static int sparx5_rr_netdevice_event(struct notifier_block *nb,
> +				     unsigned long event, void *ptr)
> +{
> +	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +	unsigned char mac[ETH_ALEN] __aligned(2);
> +	struct sparx5_router *router;
> +	struct sparx5 *sparx5;
> +
> +	ASSERT_RTNL();
> +
> +	router = container_of(nb, struct sparx5_router, netdevice_nb);
> +	sparx5 = router->sparx5;
> +
> +	/* Allow single bridge. Global router leg MAC tracks bridge mac. */
> +	if (!netif_is_bridge_master(dev))
> +		return NOTIFY_OK;
> +
> +	switch (event) {
> +	case NETDEV_CHANGEADDR:
> +		ether_addr_copy(mac, dev->dev_addr);
> +		sparx5_rr_leg_base_mac_set(sparx5, mac);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}

[Severity: High]
Does this notifier miss network namespace and offload ownership checks?

The netdevice and inetaddr notifiers process events globally. In
sparx5_rr_netdevice_event(), it only checks netif_is_bridge_master(dev). The
driver doesn't verify if the bridge actually contains Sparx5 physical ports or
if it is inside the target network namespace (init_net).

An unprivileged user inside a user or network namespace could create dummy
VLAN-aware bridges and change their MAC addresses or assign IPs. This would
trigger these global notifiers, causing the driver to program physical
hardware registers, overwrite the global router leg MAC, and exhaust the
finite 511 VMID slots, potentially leading to a denial of service.