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

[Gustavo Luiz Duarte]

On Tue, Oct 27, 2026 at 9:19 AM Breno Leitao <leitao@xxxxxxxxxx> 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@xxxxxxxxx
> > Signed-off-by: Gustavo Luiz Duarte <gustavold@xxxxxxxxx>
> > ---
> >  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.


> 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.