Re: [PATCH net-next 8/9] net: sparx5: add L3 FIB, nexthop and neighbour entry 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 FIB, nexthop and neighbour entry management

Add the data path for L3 unicast route offloading for IPv4 and IPv6 FIB
and nexthop groups.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_router.c b/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> @@ -396,6 +400,14 @@ static bool sparx5_rr_fib_info_should_ignore(struct sparx5_rr_fib_info *fi)
>  		       (IPV6_ADDR_MULTICAST | IPV6_ADDR_LINKLOCAL);
>  }
>  
> +#if IS_ENABLED(CONFIG_IPV6)
> +static void sparx5_rr_rt6_release(struct fib6_info *rt)
> +{
> +	if (!rt->nh)
> +		rt->fib6_nh->fib_nh_flags &= ~RTNH_F_OFFLOAD;
> +
> +	fib6_info_release(rt);
> +}

[Severity: High]
Does this non-atomic flag modification race with the core IPv6 stack?

The driver clears RTNH_F_OFFLOAD directly from an async worker thread
without holding the rtnl_lock. Since the core networking stack updates
these flags under the rtnl_lock, could this lockless non-atomic operation
cause torn writes and overwrite state updates like RTNH_F_DEAD?

[ ... ]

> @@ -826,6 +1143,14 @@ static void sparx5_rr_neigh_entry_destroy(struct sparx5 *sparx5,
>  	kfree(entry);
>  }
>  
> +static void sparx5_rr_neigh_entry_put(struct sparx5 *sparx5,
> +				      struct sparx5_rr_neigh_entry *neigh_entry)
> +{
> +	if (neigh_entry && list_empty(&neigh_entry->nexthop_list) &&
> +	    !neigh_entry->hw_route.vrule_id_valid)
> +		sparx5_rr_neigh_entry_destroy(sparx5, neigh_entry);
> +}

[Severity: High]
Can a neighbour entry leak permanently if its subnet route is deleted while
a gateway nexthop still references it?

When the subnet route is deleted, sparx5_rr_fib_entry_destroy() sets
neigh_entry->fib_entry = NULL and disables the VCAP rule. However, if an
ARP update arrives while the gateway route still references the neighbour,
sparx5_rr_neigh_event_work() re-enables the VCAP rule by setting
vrule_id_valid to true.

Later, when the gateway route is deleted, sparx5_rr_neigh_entry_put() will
skip destroying the neighbour because vrule_id_valid is true. The neighbour
would be left with an empty nexthop_list and a NULL fib_entry, meaning it
cannot be found during teardown by sparx5_rr_fib_flush(). Does this leak
the dev_tracker and hardware VCAP rule?

[ ... ]

> @@ -2126,6 +2753,8 @@ static int sparx5_rr_fib_replace(struct sparx5 *sparx5,
>  	/* Release and allow any previous fib_info to be deleted */
>  	sparx5_rr_fib_info_put(&old_fi);
>  
> +	fib_entry->type = sparx5_rr_rtm_type2fib_type(fi_type);
> +
>  	err = sparx5_rr_entry_nexthop_group_update(sparx5, fib_entry);

[Severity: Critical]
Could this in-place type update corrupt the LPM list?

If sparx5_rr_fib_replace() changes the route type from UNICAST to LOCAL,
the entry remains in the fib_lpm4_list or fib_lpm6_list. Later, when the
route is deleted, sparx5_rr_fib_entry_destroy() calls
sparx5_rr_fib_lpm_remove(), which has this check:

static void sparx5_rr_fib_lpm_remove(struct sparx5_rr_fib_entry *fib_entry)
{
        if (fib_entry->type != SPARX5_RR_FIB_TYPE_UNICAST)
                return;

        list_del(&fib_entry->fib_lpm_node);
}

Since the type is now LOCAL, the function returns early and skips the
list_del() operation. The entry is then freed, leaving a dangling pointer
in the linked list. Conversely, replacing a LOCAL route with a UNICAST
route appears to cause a NULL pointer dereference in list_del() because
it was never added to the list.

[ ... ]

> @@ -2247,6 +2873,28 @@ static int sparx5_rr_fib6_nexthop_prune(struct sparx5 *sparx5,
> +	old_nrt6 = fib_entry->fi.fe6_info.nrt6;
> +	new_nrt6 = old_nrt6 >= f6i->nrt6 ? old_nrt6 - f6i->nrt6 : 0;
> +
> +	rt_arr = kzalloc_objs(struct fib6_info *, new_nrt6);
> +	if (!rt_arr)
> +		return -ENOMEM;
> +
> +	for (int i = 0; i < old_nrt6; i++) {
> +		struct fib6_info *fi = old_rt_arr[i];
> +
> +		if (sparx5_rr_fib6_rt_exists(f6i, fi)) {
> +			sparx5_rr_rt6_release(fi);
> +			continue;
> +		}
> +
> +		rt_arr[j++] = fi;
> +	}

[Severity: Critical]
Is there a risk of a heap buffer overflow in this loop?

The new array size is calculated as the difference between the old count
and the count of nexthops to remove. However, if the deletion request
contains nexthops that do not perfectly match the tracked nexthops,
sparx5_rr_fib6_rt_exists() will return false.

The loop then copies these non-matching elements into rt_arr. Could the
index j exceed new_nrt6 in this scenario, writing past the end of the
dynamically allocated buffer?

[ ... ]

> @@ -2939,6 +3032,12 @@ int sparx5_rr_router_init(struct sparx5 *sparx5)
>  err_unreg_inet_addr_val_notifier:
>  	unregister_inetaddr_validator_notifier(&r->inetaddr_valid_nb);
>  err_unreg_inet_notifier:
>  	unregister_inetaddr_notifier(&r->inetaddr_nb);
> +err_unreg_fib_notifier:
> +	unregister_fib_notifier(&init_net, &r->fib_nb);
>  err_workqueue_destroy:
>  	destroy_workqueue(r->sparx5_router_owq);
> +err_fib_ht_destroy:
> +	rhashtable_destroy(&r->fib_ht);
> +err_neigh_ht_destroy:
> +	rhashtable_destroy(&r->neigh_ht);

[Severity: High]
Does this error path leak all offloaded route hardware and software
structures?

If an error occurs after register_fib_notifier() has successfully run, the
error path calls destroy_workqueue(). This allows the initial FIB dump to
finish populating routes into the hardware and the r->fib_ht structure.

However, the error path subsequently calls rhashtable_destroy() without
first calling sparx5_rr_fib_flush(). Since rhashtable_destroy() only frees
the hash buckets and not the elements, does this permanently leak all the
inserted fib_entry objects, their dev_trackers, and their hardware VCAP
rules upon a module load failure?