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")
Reviewed-by: Grant Grundler <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---
In response to v1 Paolo questioned the wisdom of grabbing the
rtnl_lock in the USB pre_reset() and releasing it in the USB
post_reset() [1]. While his concern is a legitimate one because this
looks a bit fragile, I'm still of the belief that the current patch is
the best solution.
This patch has been tested with lockdep and I saw no splats about
it. I've also read through the usb core code twice and I don't see any
way that post_reset() won't be called if pre_reset() was called,
assuming that the pre_reset() doesn't return an error (we never return
an error from pre_reset()).
If folks have some example of something that's broken by the current
rtnl_lock strategy used by this patch (or if folks feel very strongly
that it needs to be changed) then I can spin another version. ...but
as per my reply to Paolo [2] I think that does have some minor
downsides.
[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/CAD=FV=VqZq33eLiFPNiZCJmewQ1hxECmUnwbjVbvdJiDkQMAJA@mail.gmail.com
(no changes since v2)
Changes in v2:
- Added "after the cut" notes about rtnl lock strategy.
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.rc1.413.gea7ed67945-goog
Delay loops in r8152 should break out if RTL8152_INACCESSIBLE is set
so that they don't delay too long if the device becomes
inaccessible. Add the break to the loop in r8153_aldps_en().
Fixes: 4214cc550bf9 ("r8152: check if disabling ALDPS is finished")
Reviewed-by: Grant Grundler <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---
(no changes since v2)
Changes in v2:
- Added Fixes tag to RTL8152_INACCESSIBLE patches.
- Split RTL8152_INACCESSIBLE patches by the commit the loop came from.
drivers/net/usb/r8152.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3958eb622d47..fcdc9ba0f826 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -5803,6 +5803,8 @@ static void r8153_aldps_en(struct r8152 *tp, bool enable)
data &= ~EN_ALDPS;
ocp_reg_write(tp, OCP_POWER_CFG, data);
for (i = 0; i < 20; i++) {
+ if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+ return;
usleep_range(1000, 2000);
if (ocp_read_word(tp, MCU_TYPE_PLA, 0xe000) & 0x0100)
break;
--
2.43.0.rc1.413.gea7ed67945-goog
Douglas Anderson <[email protected]>
> Sent: Thursday, November 30, 2023 5:25 AM
[...]
>
> Delay loops in r8152 should break out if RTL8152_INACCESSIBLE is set
> so that they don't delay too long if the device becomes
> inaccessible. Add the break to the loop in r8153_aldps_en().
>
> Fixes: 4214cc550bf9 ("r8152: check if disabling ALDPS is finished")
> Reviewed-by: Grant Grundler <[email protected]>
> Signed-off-by: Douglas Anderson <[email protected]>
Acked-by: Hayes Wang <[email protected]>
Best Regards,
Hayes
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
On Wed, 29 Nov 2023 13:25:20 -0800 you 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).
>
> [...]
Here is the summary with links:
- [net,v3,1/5] r8152: Hold the rtnl_lock for all of reset
https://git.kernel.org/netdev/net/c/e62adaeecdc6
- [net,v3,2/5] r8152: Add RTL8152_INACCESSIBLE checks to more loops
https://git.kernel.org/netdev/net/c/32a574c7e268
- [net,v3,3/5] r8152: Add RTL8152_INACCESSIBLE to r8156b_wait_loading_flash()
https://git.kernel.org/netdev/net/c/8a67b47fced9
- [net,v3,4/5] r8152: Add RTL8152_INACCESSIBLE to r8153_pre_firmware_1()
https://git.kernel.org/netdev/net/c/8c53a7bd7065
- [net,v3,5/5] r8152: Add RTL8152_INACCESSIBLE to r8153_aldps_en()
https://git.kernel.org/netdev/net/c/79321a793945
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html