Re: [PATCH net-next v2 9/9] net: sparx5: add neighbour event handling for L3 routing

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Mon, 2026-08-17 at 14:14 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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: sparx5: add neighbour event handling for L3 routing
> 
> This adds a netevent notifier to sparx5 that queues NETEVENT_NEIGH_UPDATE
> events to the router ordered workqueue, where the resolved MAC is written
> into the driver neigh entry and pushed to the hardware ARP table and to
> all linked nexthops. Unreachable neighbours tear down the hardware ARP
> entry and mark the nexthops as unresolved.
> 
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_router.c b/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> > index 6d40d651180e6..5f6add84b0d6c 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_router.c
> 
> [ ... ]
> 
> > @@ -2419,6 +2430,107 @@ static int sparx5_rr_fib_event(struct notifier_block *nb, unsigned long event,
> >       return NOTIFY_BAD;
> >  }
> > 
> > +static void sparx5_rr_neigh_event_work(struct work_struct *work)
> > +{
> > +     struct sparx5_rr_netevent_work *net_work =
> > +             container_of(work, struct sparx5_rr_netevent_work, work);
> > +     unsigned char hwaddr[ETH_ALEN] __aligned(2);
> > +     struct sparx5 *sparx5 = net_work->sparx5;
> > +     struct neighbour *n = net_work->neigh;
> > +     struct sparx5_rr_neigh_key key = { };
> > +     struct sparx5_rr_neigh_entry *entry;
> > +     bool entry_connected;
> > +     u8 nud_state, dead;
> > +
> > +     sparx5_rr_nb2neigh_key(n, &key);
> > +
> > +     /* Frames with link-local dip are trapped, so ignore the neighbour. */
> > +     if (key.iaddr.version == SPARX5_IPV6 &&
> > +         ipv6_addr_type(&key.iaddr.ipv6) & IPV6_ADDR_LINKLOCAL)
> > +             goto out;
> 
> [Severity: Medium]
> Does the comment's justification cover neighbours that are used as
> gateways rather than as destinations?
> 
> The trap rule installed by sparx5_rr_lpm_link_local_create() matches on
> destination fe80::/64 only, and sparx5_rr_fib_info_should_ignore() only
> skips routes whose destination is link-local:
> 
>         return fi->version == SPARX5_IPV6 &&
>                ipv6_addr_type(&fi->fe6_info.rt_arr[0]->fib6_dst.addr) &
>                        (IPV6_ADDR_MULTICAST | IPV6_ADDR_LINKLOCAL);
> 
> A route such as "ip -6 route add 2001:db8::/64 via fe80::1 dev vlanX" is
> therefore still offloaded, and sparx5_rr_nexthop_init() sets
> nh->neigh_tbl = &nd_tbl for every AF_INET6 gateway without a link-local
> filter, so sparx5_rr_nexthop_neigh_init() creates a neigh entry for
> fe80::1.
> 
> Since this work handler is the only writer of entry->hwaddr, can such a
> nexthop ever obtain a MAC?  sparx5_rr_nexthop_egress_derive() would keep
> nh->trapped set and program the zero MAC, so routes via a link-local
> gateway (including RA-derived default routes) stay trapped to the CPU
> while being reported as offloaded.
> 


No, it does not cover that case. A link-local destination must not be
forwareded which the fe80::/64 trap rule takes care of. But a link-local
gateway must be resolved, because its MAC is what a nexthop egresses with.
Skipping the neighbour entirely satisfies the first and breaks the second.

> > +
> > +     /* If n changes after this read section, we will get another neigh
> > +      * event, which is processed after the current one.
> > +      */
> > +     read_lock_bh(&n->lock);
> > +     ether_addr_copy(hwaddr, n->ha);
> > +     nud_state = n->nud_state;
> > +     dead = n->dead;
> > +     read_unlock_bh(&n->lock);
> > +
> > +     mutex_lock(&sparx5->router->lock);
> > +
> > +     entry_connected = nud_state & NUD_VALID && !dead;
> > +     entry = sparx5_rr_neigh_entry_lookup(sparx5, &key);
> > +     if (!entry_connected && !entry)
> > +             goto out_mutex;
> > +
> > +     if (!entry) {
> > +             entry = sparx5_rr_neigh_entry_create(sparx5, &key);
> > +             if (IS_ERR(entry))
> > +                     goto out_mutex;
> > +     }
> 
> [Severity: Medium]
> With this notifier being the only source of entry->hwaddr, how does an
> already-resolved kernel neighbour get synchronized into a newly created
> driver entry?
> 
> sparx5_rr_nexthop_neigh_init() only kicks ARP/NDP when the kernel
> neighbour did not already exist, and never samples n->ha / n->nud_state:
> 
>         n = neigh_lookup(nh->neigh_tbl, &nh->gw_addr, dev);
>         if (!n) {
>                 n = neigh_create(nh->neigh_tbl, &nh->gw_addr, dev);
>                 if (IS_ERR(n))
>                         return PTR_ERR(n);
>                 /* Start arp process */
>                 neigh_event_send(n, NULL);
>         }
> 
> For a NUD_PERMANENT neighbour ("ip neigh add <gw> lladdr <mac> nud
> permanent dev vlanX") no further NETEVENT_NEIGH_UPDATE is generated -
> net/core/neighbour.c only emits netevents from __neigh_update(),
> neigh_timer_handler() and neigh_cleanup_and_release(), and permanent
> entries have no timer and are skipped by the GC work.  Does that leave
> entry->hwaddr zero forever, so sparx5_rr_nexthop_egress_derive() keeps
> nh->trapped set while the FIB entry is reported as offloaded?
> 
> The same applies when the event arrives before a router leg exists:
> sparx5_rr_neigh_entry_create() returns ERR_PTR(-EINVAL) from
> 
>         leg = sparx5_rr_leg_find_by_dev(sparx5, key->dev);
>         if (!leg)
>                 return ERR_PTR(-EINVAL);
> 
> and the event is dropped with no replay when the leg later appears.
> 
> For comparison, mlxsw_sp_nexthop_neigh_init() reads nud_state/dead under
> n->lock and calls __mlxsw_sp_nexthop_neigh_update() at init time:
> 
>         read_lock_bh(&n->lock);
>         nud_state = n->nud_state;
>         dead = n->dead;
>         read_unlock_bh(&n->lock);
>         __mlxsw_sp_nexthop_neigh_update(nh, !(nud_state & NUD_VALID && !dead));
> 


It is right, nothing else writes entry->hwaddr, so it stays zero and the nexthop
is trapped forever. I will fix this by sampling at init as mlxsw does, after the
nexthop is linked into neigh_entry->nexthop_list, so the update
reaches the nexthop that just attached.


> > +
> > +     if (entry->connected && entry_connected &&
> > +         ether_addr_equal(entry->hwaddr, hwaddr))
> > +             goto out_mutex;
> 
> [Severity: Medium]
> Can this early exit permanently block a retry after a failed hardware
> write?
> 
> sparx5_rr_neigh_entry_update() marks the entry connected before the
> fallible hardware apply and only downgrades the offload marking:
> 
>         entry->connected = adding;
> 
>         if (adding) {
>                 err = sparx5_rr_neigh_entry_hw_apply(sparx5, entry);
>                 if (err)
>                         offloaded = false;
> 
> sparx5_rr_lpm_arp_entry_mod() can fail while the rule keeps the previous
> DMAC:
> 
>         vrule = vcap_get_rule(vctrl, vrule_id);
>         if (IS_ERR(vrule))
>                 return -EINVAL;
> 
> and sparx5_rr_lpm_arp_entry_create() can fail on rule allocation or
> vcap_val_add_rule().
> 
> Since the new MAC has already been stored in entry->hwaddr and
> entry->connected is true, every following NUD_VALID confirmation
> (REACHABLE/STALE/DELAY) hits the test above and skips both the LPM/ARP
> update and sparx5_rr_nexthops_update_notify().  For an IP that moves to a
> new MAC (VRRP failover, VM migration), would hardware keep rewriting DMAC
> to the old MAC with no trap and no further retry?
> 
> Would keying the skip on hw_route.vrule_id_valid (or propagating the
> error from sparx5_rr_neigh_entry_update()) avoid that?
> 

Yes, it can. I will record the outcome rather than key on vrule_id_valid,
because that flag is not a reliable proxy any more. A link-local neighbour
now deliberately has no host route, so vrule_id_valid stays false for it
and the skip would never trigger. Recording the state enables the next
event to drive the retry.

> > +
> > +     ether_addr_copy(entry->hwaddr, hwaddr);
> > +     sparx5_rr_neigh_entry_update(sparx5, entry, entry_connected);
> > +     sparx5_rr_nexthops_update_notify(sparx5, entry, entry_connected);
> > +     if (!entry_connected)
> > +             sparx5_rr_neigh_entry_put(sparx5, entry);
> 
> [Severity: High]
> Should the MAC be committed into entry->hwaddr on the disconnect path as
> well?
> 
> The kernel does not clear n->ha when a neighbour goes to NUD_FAILED or
> dead - neigh_invalidate() only flushes arp_queue, and neigh->ha is only
> overwritten when a new lladdr is supplied.  So for !entry_connected this
> stores the last, now invalid MAC while entry->connected becomes false, and
> nothing else ever zeroes entry->hwaddr.  sparx5_rr_neigh_entry_put() keeps
> the entry alive while a nexthop still references it.
> 
> The only other consumer derives "resolved" purely from the MAC being
> non-zero and ignores entry->connected:
> 
>         nh->trapped = !nh_neigh || is_zero_ether_addr(nh_neigh->hwaddr);
> 
>         if (nh_neigh) {
>                 memcpy(mac, nh_neigh->hwaddr, ETH_ALEN);
> 
> On ordinary route churn, sparx5_rr_entry_nexthop_group_update() builds and
> programs the new group before releasing the old one:
> 
>         /* Prepare new group in SW representation */
>         new_nh_grp = sparx5_rr_nexthop_group_create(sparx5, fib_entry);
>         ...
>         err = sparx5_rr_fib_entry_hw_apply(sparx5, fib_entry);
>         ...
>         sparx5_rr_nexthop_group_put(sparx5, old_nh_grp);
> 
> so sparx5_rr_nexthop_neigh_init() re-attaches to the surviving entry and
> sparx5_rr_nexthop_egress_derive() then clears nh->trapped and installs the
> dead MAC.  Does that leave traffic forwarded to a host that no longer
> exists instead of trapped, with no path back to trapping since the kernel
> never sees the traffic?
> 
> This also seems to differ from the commit message wording "mark linked
> nexthops as unresolved so traffic traps to the CPU".
> 

No, it should not. In v3 I will only commit an address when told it is valid.
And sparx5_rr_nexthop_egress_derive() no longer treats a non-zero hwaddr as
proof of resolution, not emits an address when it has decided the nexthop is
trapped.

> > +
> > +out_mutex:
> > +     mutex_unlock(&sparx5->router->lock);
> > +out:
> > +     neigh_release(n);
> > +     kfree(net_work);
> > +}
> > +
> > +/* Handle neighbour update events. Used to manage neigh_entries. Called in
> > + * atomic context, with rcu_read_lock().
> > + */
> > +static int sparx5_rr_netevent_event(struct notifier_block *nb,
> > +                                 unsigned long event, void *ptr)
> > +{
> 
> [ ... ]
> 
> > +             net_work = kzalloc_obj(*net_work, GFP_ATOMIC);
> > +             if (!net_work)
> > +                     return NOTIFY_BAD;
> 
> [Severity: Low]
> Would NOTIFY_DONE be a better return value here?
> 
> NETEVENT_NEIGH_UPDATE is delivered over the single system-wide atomic
> chain:
> 
> net/core/netevent.c:call_netevent_notifiers() {
>         return atomic_notifier_call_chain(&netevent_notif_chain, val, v);
> }
> 
> and NOTIFY_BAD carries NOTIFY_STOP_MASK, which ends the traversal:
> 
> kernel/notifier.c:notifier_call_chain() {
>         ...
>                 if (ret & NOTIFY_STOP_MASK)
>                         break;
>         ...
> }
> 
> So a driver-private GFP_ATOMIC failure suppresses the notification for
> every listener registered after sparx5.  The same idiom exists in
> mlxsw_sp_router_schedule_work(), so this is minor, but the failure looks
> purely local and recoverable here.
> 

Sure, I will change this to NOTIFY_DONE.

> > +
> > +             INIT_WORK(&net_work->work, sparx5_rr_neigh_event_work);
> > +             net_work->sparx5 = router->sparx5;
> > +             net_work->neigh = neigh_clone(n);
> > +             net_work->event = event;
> > +             sparx5_rr_schedule_work(router->sparx5, &net_work->work);
> > +
> > +             return NOTIFY_DONE;
> > +     }
> > +
> > +     return NOTIFY_DONE;
> > +};
> > +
> 
> [ ... ]
> 
> > @@ -2855,6 +2972,8 @@ int sparx5_rr_router_init(struct sparx5 *sparx5)
> >       unregister_inetaddr_notifier(&r->inetaddr_nb);
> >  err_unreg_fib_notifier:
> >       unregister_fib_notifier(&init_net, &r->fib_nb);
> > +err_unreg_netevent_notifier:
> > +     unregister_netevent_notifier(&r->netevent_nb);
> >  err_workqueue_destroy:
> >       destroy_workqueue(r->sparx5_router_owq);
> >       sparx5_rr_fib_flush(sparx5);
> 
> [Severity: Low]
> While looking at this unwind path, the neighbour hashtable is destroyed
> before the legs that own entries in it are flushed:
> 
> err_neigh_ht_destroy:
>         rhashtable_destroy(&r->neigh_ht);
> err_blackhole_destroy:
>         sparx5_rr_router_legs_flush(sparx5);
> 
> sparx5_rr_router_leg_destroy() walks leg->neigh_list and reaches
> rhashtable_remove_fast() via sparx5_rr_neigh_entry_destroy() ->
> sparx5_rr_neigh_entry_remove(), on a table whose bucket table has already
> been freed.  sparx5_rr_router_deinit() uses the opposite order (legs
> flush, WARN_ON(nelems), then rhashtable_destroy).
> 
> The dangerous state does not appear reachable today: neigh entries only
> exist once a device leg exists, and legs are only created from the
> inetaddr/inet6addr handlers registered after register_fib_notifier(),
> which is the only registration here that realistically fails;
> sparx5_rr_netdevice_event() always returns NOTIFY_OK, and the
> inet6addr registrations only fail on duplicate registration.  This
> ordering also comes from the earlier patch in the series that added the
> FIB/nexthop/neighbour entry management, not from this patch, which only
> adds one more producer of neigh entries.  Would it still be worth
> matching the deinit order defensively?

Yes, worth doing, and the reachability analysis matches mine. I will change
this.