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

[Doug Anderson]

Hi,

On Tue, Nov 21, 2023 at 2:25 AM Paolo Abeni <pabeni@xxxxxxxxxx> 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