[1/2] r8152: Hold the rtnl_lock for all of reset

  As of commit d9962b0d4202 ("r8152: Block future register access if
register access fails") there is a race condition that can happen
between the USB device reset thread and napi_enable() (not) getting
called during rtl8152_open(). Specifically:
* While rtl8152_open() is running we get a register access error
  that's _not_ -ENODEV and queue up a USB reset.
* rtl8152_open() exits before calling napi_enable() due to any reason
  (including usb_submit_urb() returning an error).

In that case:
* Since the USB reset is perform in a separate thread asynchronously,
  it can run at anytime USB device lock is not held - even before
  rtl8152_open() has exited with an error and caused __dev_open() to
  clear the __LINK_STATE_START bit.
* The rtl8152_pre_reset() will notice that the netif_running() returns
  true (since __LINK_STATE_START wasn't cleared) so it won't exit
  early.
* rtl8152_pre_reset() will then hang in napi_disable() because
  napi_enable() was never called.

We can fix the race by making sure that the r8152 reset routines don't
run at the same time as we're opening the device. Specifically we need
the reset routines in their entirety rely on the return value of
netif_running(). The only way to reliably depend on that is for them
to hold the rntl_lock() mutex for the duration of reset.

Grabbing the rntl_lock() mutex for the duration of reset seems like a
long time, but reset is not expected to be common and the rtnl_lock()
mutex is already held for long durations since the core grabs it
around the open/close calls.

Fixes: d9962b0d4202 ("r8152: Block future register access if register access fails")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/net/usb/r8152.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
  

On Fri, Nov 17, 2023 at 1:10 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> As of commit d9962b0d4202 ("r8152: Block future register access if
> register access fails") there is a race condition that can happen
> between the USB device reset thread and napi_enable() (not) getting
> called during rtl8152_open(). Specifically:
> * While rtl8152_open() is running we get a register access error
>   that's _not_ -ENODEV and queue up a USB reset.
> * rtl8152_open() exits before calling napi_enable() due to any reason
>   (including usb_submit_urb() returning an error).
>
> In that case:
> * Since the USB reset is perform in a separate thread asynchronously,
>   it can run at anytime USB device lock is not held - even before
>   rtl8152_open() has exited with an error and caused __dev_open() to
>   clear the __LINK_STATE_START bit.
> * The rtl8152_pre_reset() will notice that the netif_running() returns
>   true (since __LINK_STATE_START wasn't cleared) so it won't exit
>   early.
> * rtl8152_pre_reset() will then hang in napi_disable() because
>   napi_enable() was never called.
>
> We can fix the race by making sure that the r8152 reset routines don't
> run at the same time as we're opening the device. Specifically we need
> the reset routines in their entirety rely on the return value of
> netif_running(). The only way to reliably depend on that is for them
> to hold the rntl_lock() mutex for the duration of reset.
>
> Grabbing the rntl_lock() mutex for the duration of reset seems like a
> long time, but reset is not expected to be common and the rtnl_lock()
> mutex is already held for long durations since the core grabs it
> around the open/close calls.
>
> Fixes: d9962b0d4202 ("r8152: Block future register access if register access fails")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Grant Grundler <grundler@chromium.org>

BTW, for ChromeOS systems, the outcome of hang in napi_disable() is a
"hung task" panic after 120 seconds. Fortunately, the stack trace made
it relatively easy (compared to other hung tasks I've looked at) to
unravel.

Doug gets all the credit for figuring out this solution (using rtnl_lock()).

cheers,
grant

> ---
>
>  drivers/net/usb/r8152.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 2c5c1e91ded6..d6edf0254599 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -8397,6 +8397,8 @@ static int rtl8152_pre_reset(struct usb_interface *intf)
>         struct r8152 *tp = usb_get_intfdata(intf);
>         struct net_device *netdev;
>
> +       rtnl_lock();
> +
>         if (!tp || !test_bit(PROBED_WITH_NO_ERRORS, &tp->flags))
>                 return 0;
>
> @@ -8428,20 +8430,17 @@ static int rtl8152_post_reset(struct usb_interface *intf)
>         struct sockaddr sa;
>
>         if (!tp || !test_bit(PROBED_WITH_NO_ERRORS, &tp->flags))
> -               return 0;
> +               goto exit;
>
>         rtl_set_accessible(tp);
>
>         /* reset the MAC address in case of policy change */
> -       if (determine_ethernet_addr(tp, &sa) >= 0) {
> -               rtnl_lock();
> +       if (determine_ethernet_addr(tp, &sa) >= 0)
>                 dev_set_mac_address (tp->netdev, &sa, NULL);
> -               rtnl_unlock();
> -       }
>
>         netdev = tp->netdev;
>         if (!netif_running(netdev))
> -               return 0;
> +               goto exit;
>
>         set_bit(WORK_ENABLE, &tp->flags);
>         if (netif_carrier_ok(netdev)) {
> @@ -8460,6 +8459,8 @@ static int rtl8152_post_reset(struct usb_interface *intf)
>         if (!list_empty(&tp->rx_done))
>                 napi_schedule(&tp->napi);
>
> +exit:
> +       rtnl_unlock();
>         return 0;
>  }
>
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
  

On Fri, 2023-11-17 at 13:08 -0800, Douglas Anderson wrote:
> As of commit d9962b0d4202 ("r8152: Block future register access if
> register access fails") there is a race condition that can happen
> between the USB device reset thread and napi_enable() (not) getting
> called during rtl8152_open(). Specifically:
> * While rtl8152_open() is running we get a register access error
>   that's _not_ -ENODEV and queue up a USB reset.
> * rtl8152_open() exits before calling napi_enable() due to any reason
>   (including usb_submit_urb() returning an error).
> 
> In that case:
> * Since the USB reset is perform in a separate thread asynchronously,
>   it can run at anytime USB device lock is not held - even before
>   rtl8152_open() has exited with an error and caused __dev_open() to
>   clear the __LINK_STATE_START bit.
> * The rtl8152_pre_reset() will notice that the netif_running() returns
>   true (since __LINK_STATE_START wasn't cleared) so it won't exit
>   early.
> * rtl8152_pre_reset() will then hang in napi_disable() because
>   napi_enable() was never called.
> 
> We can fix the race by making sure that the r8152 reset routines don't
> run at the same time as we're opening the device. Specifically we need
> the reset routines in their entirety rely on the return value of
> netif_running(). The only way to reliably depend on that is for them
> to hold the rntl_lock() mutex for the duration of reset.

Acquiring the rtnl_lock in a callback and releasing it in a different
one, with the latter called depending on the configuration, looks
fragile and possibly prone to deadlock issues.

Have you tested your patch with lockdep enabled?

Can you instead acquire the rtnl lock only for pre_reset/post_rest and
in rtl8152_open() do something alike:

	for (i = 0; i < MAX_WAIT; ++i) {
		if (usb_lock_device_for_reset(udev, NULL))
			goto error;

		wait_again = udev->reset_in_progress;
		usb_unlock_device(udev);
		if (!wait_again)
			break;

		usleep(1);
	}
	if (i == MAX_WAIT)
		goto error;

which should be more polite to other locks?


Thanks,

Paolo
  

Hi,

On Tue, Nov 21, 2023 at 2:25 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2023-11-17 at 13:08 -0800, Douglas Anderson wrote:
> > As of commit d9962b0d4202 ("r8152: Block future register access if
> > register access fails") there is a race condition that can happen
> > between the USB device reset thread and napi_enable() (not) getting
> > called during rtl8152_open(). Specifically:
> > * While rtl8152_open() is running we get a register access error
> >   that's _not_ -ENODEV and queue up a USB reset.
> > * rtl8152_open() exits before calling napi_enable() due to any reason
> >   (including usb_submit_urb() returning an error).
> >
> > In that case:
> > * Since the USB reset is perform in a separate thread asynchronously,
> >   it can run at anytime USB device lock is not held - even before
> >   rtl8152_open() has exited with an error and caused __dev_open() to
> >   clear the __LINK_STATE_START bit.
> > * The rtl8152_pre_reset() will notice that the netif_running() returns
> >   true (since __LINK_STATE_START wasn't cleared) so it won't exit
> >   early.
> > * rtl8152_pre_reset() will then hang in napi_disable() because
> >   napi_enable() was never called.
> >
> > We can fix the race by making sure that the r8152 reset routines don't
> > run at the same time as we're opening the device. Specifically we need
> > the reset routines in their entirety rely on the return value of
> > netif_running(). The only way to reliably depend on that is for them
> > to hold the rntl_lock() mutex for the duration of reset.
>
> Acquiring the rtnl_lock in a callback and releasing it in a different
> one, with the latter called depending on the configuration, looks
> fragile and possibly prone to deadlock issues.

Yeah, I debated this as well. I looked through the USB code and I
couldn't find any reason that it wouldn't work to hold the lock for
the duration. I agree that it's a little more fragile in one sense,
but I think it avoids potential races too and that makes it less
fragile in a different sense. ;-)


> Have you tested your patch with lockdep enabled?

Yes, lockdep reported no problems with my patch. Indeed lockdep hints
are how I ended up with the current solution. When I originally tried
to lock the device in rtl8152_open() then lockdep yelled at me about
the AB BA issues between the device lock and the rtnl_lock() mutex
which made me realize that grabbing the rtnl_lock() in the reset code
was the right solution here.


> Can you instead acquire the rtnl lock only for pre_reset/post_rest and
> in rtl8152_open() do something alike:
>
>         for (i = 0; i < MAX_WAIT; ++i) {
>                 if (usb_lock_device_for_reset(udev, NULL))
>                         goto error;
>
>                 wait_again = udev->reset_in_progress;
>                 usb_unlock_device(udev);
>                 if (!wait_again)
>                         break;
>
>                 usleep(1);
>         }
>         if (i == MAX_WAIT)
>                 goto error;
>
> which should be more polite to other locks?

Right, I could add a call to usb_lock_device_for_reset() here. That
shouldn't trigger AB BA lockdep splats since it has a timeout. I'm not
100% convinced that it's right, though. ...and I'm fairly certain that
if we call it we don't want to call it in a loop.

I don't think we should have a loop because
usb_lock_device_for_reset() already has a loop in it and I don't think
an extra loop will help. I'd imagine that usb_lock_device_for_reset()
would usually timeout only if USB reset is currently running and
somehow blocked. If pre_reset or post_reset are currently running then
they've already got the USB lock (from their caller) and may be
blocked waiting for the rtnl_lock. We've already got the rtnl_lock
(from our caller) and now we're waiting for the USB lock. In neither
case do I think it's a good idea to drop the locks that our caller
grabbed for us, so about the best we can do in that case is return an
error from r8152_open() after the first timeout.

Let's step back and think about why we might want to get the USB lock
in the first place. This would only be necessary if we dropped the
lock between pre_reset and post_reset, right? ...so we're trying to
make sure that we're not trying to open a device while the USB reset
code is half executed. I guess the expected order of operations we're
trying to protect against would be:

1. rtl8152_close() is called and has a transfer error that queues up a reset.
2. USB reset starts and pre-reset runs. It should be a no-op because
netif_running() would return false.
3. rl8152_open() is called and opens the device successfully
4. USB reset runs post-reset, which is no longer the inverse of
pre-reset because netif_running() would return true. This would end up
with, among other things, an unbalanced napi_enable() count.

That feels relatively unlikely to actually hit but it does seem
conceivably possible. Thus if we do drop the rtnl_lock between
pre-reset and post-reset then I agree we should call
usb_lock_device_for_reset(). Probably we need to do that for _both_
rtl8152_open() and rtl8152_close()? We also probably don't need to
hold the lock for the whole duration of rtl8152_open() /
rtl8152_close(). We can just grab it and release it to make sure that
we're not midway through a reset.

I guess one sorta odd thing here is that it means that rtl8152_close()
could now fail if someone called it at just the right time and we were
unable to grab the USB lock. Though it does have an error return,
that's not a failure that I'd expect most users to be able to handle
terribly well. I guess conceivably we could return -EAGAIN or
-EDEADLOCK in this case, but ick...


Hopefully the above makes sense. I'd be interested to hear your
further thoughts on the issue. I'd still lean towards leaving the code
as-is and holding the rtnl_lock across the whole reset, but for all
practical purposes I think it would be fine to split it and add
usb_lock_device_for_reset() to the rtl8152_open() / rtl8152_close(),
since the issues I talk about above seem like they'd need extremely
rare timing conditions to hit.

-Doug