Re: [net-next,5/8] net: core: neighbour: Inline neigh_update_notify() calls

[Jakub Kicinski <kuba@kernel.org>]

I'm a bit behind so I haven't validated this myself TBH,
but in the interest of time perhaps you can TAL?

This is an AI-generated review of your patch.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: core: neighbour: Make another netlink notification atomically

This commit moves the netlink notification call in neigh_timer_handler()
earlier, into the critical section before neigh_probe() releases the lock,
to fix a race where notifications could reflect stale state.

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 635d71c6420f..5512dd7035b1 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c

[ ... ]

Earlier in the function, when max probes are exhausted, there is this code:

        if ((neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) &&
            atomic_read(&neigh->probes) >= neigh_max_probes(neigh)) {
                ...
                notify = 1;
                goto out;
        }

Then the new code places __neigh_notify() here:

> @@ -1180,6 +1180,10 @@ static void neigh_timer_handler(struct timer_list *t)
>  		if (!mod_timer(&neigh->timer, next))
>  			neigh_hold(neigh);
>  	}
> +
> +	if (notify)
> +		__neigh_notify(neigh, RTM_NEWNEIGH, 0, 0);
> +
>  	if (neigh->nud_state & (NUD_INCOMPLETE | NUD_PROBE)) {
>  		neigh_probe(neigh);
>  	} else {
> @@ -1187,10 +1191,8 @@ static void neigh_timer_handler(struct timer_list *t)
> out:
>  		write_unlock(&neigh->lock);
>  	}
> -
> -	if (notify) {
> -		neigh_notify(neigh, RTM_NEWNEIGH, 0, 0);
> +	if (notify)
>  		call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, neigh);
> -	}

When the neighbor exhausts max probes and transitions to NUD_FAILED or
NUD_STALE, the code sets notify = 1 and then does goto out. This jumps
directly to the out: label, skipping the new __neigh_notify() call.

The old code called neigh_notify() after the out: label, so the netlink
notification was always sent when notify was set. With this change, the
goto out path no longer sends the netlink notification to userspace,
though it still calls call_netevent_notifiers() for kernel-internal
listeners.

Is this an oversight? Should the __neigh_notify() call be placed before the
goto out, or should the goto out path have its own __neigh_notify() call?