Re: [PATCH net 1/1] udp: diag: bound bucket lock hold time

[zihan xi]

On Sun, Sep 6, 2026 at 3:02 AM Simon Horman <horms@xxxxxxxxxx> wrote:
>
> On Sat, Sep 05, 2026 at 11:49:05PM +0800, zihan xi wrote:
> > On Sat, Sep 5, 2026 at 11:12 PM Simon Horman <horms@xxxxxxxxxx> wrote:
> > >
> > > This is an AI-generated review of your patch. The human sending this
> > > email has considered the AI review valid, or at least plausible.
> > > Full review at: https://sashiko.dev/#/patchset/133b6aee9e2c908c9da37d5585b3d2cd016906cd.1788187473.git.zihanx@xxxxxxxxxx?part=1
> > > ---
> > > udp: diag: bound bucket lock hold time
> > >
> > > This patch limits the time the UDP hash bucket spinlock is held during
> > > netlink socket dumps by collecting a bounded batch of matching sockets.
> > > It takes a reference to each socket, releases the lock, processes the
> > > batch, and resumes from a preserved cursor.
> > >
> > > > diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
> > > > index f4b24e628cf8d..18cfe0df654dd 100644
> > > > --- a/net/ipv4/udp_diag.c
> > > > +++ b/net/ipv4/udp_diag.c
> > > [ ... ]
> > > > +static void udp_diag_dump_done(struct netlink_callback *cb)
> > > > +{
> > > > +     struct inet_diag_dump_data *cb_data = cb->data;
> > > > +     struct sock *sk = (struct sock *)cb->args[2];
> > > > +
> > > > +     if (sk) {
> > > > +             cb->args[2] = 0;
> > > > +             sock_put(sk);
> > > > +     }
> > > > +     cb_data->dump_done = NULL;
> > > > +     module_put(THIS_MODULE);
> > > > +}
> > >
> > > [Severity: High]
> > > Does this module_put() introduce a race condition with module unloading?
> > >
> > > When an active netlink dump for UDP sockets finishes, inet_diag_dump_done()
> > > invokes udp_diag_dump_done(). This function drops its module reference using
> > > module_put(THIS_MODULE) as its final step.
> > >
> > > If this was the last reference, a concurrent rmmod udp_diag operation can
> > > proceed and unmap the module's text segment before the thread executing
> > > udp_diag_dump_done() executes its return instruction to return to inet_diag.
> > >
> > > Could this result in a kernel panic due to the CPU attempting to execute
> > > unmapped memory?
> >
> > Yes.  If that module_put() drops the last reference, rmmod can unmap
> > udp_diag while dump_done() is still returning to inet_diag.
> >
> > I will keep the extra module_get() in udp_diag_dump(), and move the
> > matching module_put() into inet_diag_dump_done() so it runs after the
> > callback has returned.
>
> I think that the fundamental problem here is that managing module lifecycle
> from within the module itself tends to be unsafe. I'm not sure your
> proposal addresses that problem.
>
You are right.  Keeping __module_get() in udp_diag and only moving the
put into inet_diag_dump_done() still lets the module pin itself.

> I think one possible solution is to move the module_put into delayed work.
> And another, is to move module lifecycle handling into the core.
>
> Possibly the first option is cleaner as I think that only udp_diag
> has the need for this.
>
The extra pin exists only because the dump kept a socket cursor after
inet_diag had already dropped the handler.  I will drop that cursor
instead of adding delayed work or extra core lifecycle state.

v2 will bound the bucket lock the way tcp_diag already batches, and
leave module get/put in inet_diag_lock_handler() /
inet_diag_unlock_handler().

Thanks,
Zihan