Re: [PATCH net v4 3/3] netconsole: recheck device registration after waiting for carrier

[Breno Leitao <leitao@debian.org>]

On Thu, Oct 29, 2026 at 09:10:48PM +0000, Gustavo Luiz Duarte wrote:
> On Tue, Oct 27, 2026 at 9:19 AM Breno Leitao <leitao@debian.org> wrote:
> >
> > On Wed, Oct 21, 2026 at 03:19:07PM +0100, Gustavo Luiz Duarte wrote:
> > > netcons_netpoll_setup() drops RTNL while it waits for carrier when the
> > > device is not yet up. If the device is unregistered in that window, the
> > > NETDEV_UNREGISTER notifier runs before np->dev is set and has nothing
> > > to match.
> > >
> > > We might then end up setting up netpoll on a device that is being
> > > unregistered while holding a reference to it, stalling
> > > unregister_netdevice() in netdev_wait_allrefs_any().
> > >
> > > Add a check to confirm the device is still registered once we retake
> > > RTNL. Release the device reference and return -ENODEV otherwise.
> > >
> > > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-netcons-fixes-v3-1-1aac85ed7665@gmail.com
> > > Signed-off-by: Gustavo Luiz Duarte <gustavold@gmail.com>
> > > ---
> > >  drivers/net/netconsole.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index be7eb632ee42..bd36ebd334aa 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -534,6 +534,16 @@ static int netcons_netpoll_setup(struct netconsole_target *nt)
> > >               rtnl_unlock();
> > >               netcons_wait_carrier(np, ndev);
> > >               rtnl_lock();
> > > +
> > > +             /* Check that ndev hasn't been unregistered when we released
> > > +              * the RTNL above.
> > > +              */
> > > +             if (ndev->reg_state != NETREG_REGISTERED) {
> > > +                     np_err(np, "%s was unregistered while waiting for carrier, aborting\n",
> > > +                            netcons_egress_dev(np, buf, sizeof(buf)));
> > > +                     err = -ENODEV;
> > > +                     goto put;
> > > +             }
> > >       }
> >
> > I think there is a better way to do it, inside netcons_wait_carrier().
> >
> > netcons_wait_carrier() loops waiting for the carrier, and exit if in
> > time_after(). I think it will be cleaner to exit there.
> 
> Yes, I think it makes sense to bail out of netcons_wait_carrier() if
> we detect that the device is being unregistered. It doesn't replace
> the check under RTNL, though, since the device can still be
> unregistered after the loop exits and before we retake RTNL.

Agree, but we probably don't need to print inside RTNL?

> > then netcons_wait_carrier() would return a bool, and you goto 'put' in
> > both case (ndev->reg_state != NETREG_REGISTERED) or in the
> > time_after() exit.
> 
> Do we really need to check the return value of netcons_wait_carrier()?
> If it detected that the device is being unregistered, then
> (ndev->reg_state != NETREG_REGISTERED) will still be true here, since
> reg_state never goes back to NETREG_REGISTERED, no?
> 
> Or do you mean to fail netcons_netpoll_setup() not only when the
> device is being unregistered but also when the carrier times out? That
> would be a considerable behavior change, because then a slow link
> would fail netconsole completely instead of only losing the messages
> sent before the link comes up.
> 
> I'll keep this patch as the fix for the missed NETDEV_UNREGISTER and
> add a separate patch to bail out of netcons_wait_carrier() early.

Cool, thanks, bonus point for not printing (np_err) inside the lock. :-)