Re: [PATCH net] udp: restore hlist_nulls for primary and secondary hash tables

[Eric Dumazet <edumazet@google.com>]

On Wed, Sep 23, 2026 at 2:59 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 2 · Low: 2

Thanks for the review, comments below.

> [Severity: Medium]
> The pre-image here was an unconditional head insert.  Is the new IPv6 plus
> SO_REUSEPORT tail insert part of the hlist_nulls conversion?  The changelog
> only describes restoring hlist_nulls and adding the traversal restarts, so
> this chain ordering change is not mentioned.

No, it is not, and it should not have been there.  It was an attempt to
make udp_lib_rehash() consistent with udp_lib_get_port(), but it is not
needed for the hlist_nulls conversion and does not belong in this patch.

v2 keeps the unconditional head insert.  The CONFIG_BASE_SMALL
misdelivery you describe is a consequence of this hunk and goes away
with it.

> [Severity: Medium]
> Can this tail insertion defeat the nulls-value check added elsewhere in the
> patch?

Yes, and this is the better argument for dropping that hunk.

hlist_nulls_add_tail_rcu() sets n->next = last->next, that is the nulls
marker of the bucket itself, while hlist_nulls_add_head_rcu() sets
n->next = h->first.  Only the latter is compatible with the nulls
scheme: a reader parked on a node that leaves a bucket and comes back
must be sent to the head of the chain, not to its end marker.  With the
tail insert the reader stops with a matching nulls value, so the check
can not fire and the rest of the chain is silently skipped, exactly as
you describe.

v2 keeps head insertion at this site and says why.

Note I am keeping the tail insert in sk_nulls_add_node_rcu(): the
pre-image sk_add_node_rcu(), used by udp_lib_get_port(), already did
hlist_add_tail_rcu() for IPv6 SO_REUSEPORT sockets, see d894ba18d4e4
("soreuseport: fix ordering for mixed v4/v6 sockets").  udp_lib_get_port()
only inserts unhashed sockets, and for the same-bucket re-insert case it
behaves as before the conversion, since hlist_add_tail_rcu() left
n->next = NULL and readers stopped there as well.  No change in
behavior, so not something to address in a fix for net.

> [Severity: Low]
> This path now does an O(N) chain walk with softirqs disabled.

Goes away with the above.

> [Severity: High]
> Should this walk also validate the terminating nulls value?

I do not think so, and this is not a regression.

Before this patch the same loop walked a plain hlist.  A socket moved by
udp_lib_rehash() kept its ->next pointing into the new chain, so the
reader already wandered into the other bucket, stopped on that bucket's
NULL and missed the remaining listeners of the original chain, and the
hash2_any pass could already revisit sockets.  The conversion does not
make this worse, it only makes the condition detectable.

Detecting it does not help.  As you note, by the time we reach the end
of the chain skbs have been cloned and queued, so a "goto begin" restart
would deliver duplicates, which is worse than missing a listener during
a concurrent rehash.  Multicast delivery here is best effort.

v2 adds a comment at both mcast_deliver() sites to record this.

> For completeness, __udp4_lib_demux_lookup() and __udp6_lib_demux_lookup()
> were also converted to nulls iteration without the check.

These look at the first entry only and break out, and
inet_match()/inet6_match() validate the 4-tuple, so there is nothing to
restart.

> [Severity: Low]
> This isn't a bug, but .clang-format still lists only the old name in
> ForEachMacros:

Good catch, v2 renames the entry.

Thanks !

pw-bot: cr