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

[Simon Horman]

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.

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.