Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

[Johannes Berg]

Hi Guenter,

> This patch results in the attached lockdep splat when running the
> ast2600-evb emulation in qemu with aspeed_g5_defconfig and lock debugging
> enabled. Reverting this patch fixes the problem.

Umm ... That's only true if you think the problem is the lockdep splat,
rather than the actual potential deadlock?!

> [    9.809960] ======================================================
> [    9.810053] WARNING: possible circular locking dependency detected
> [    9.810196] 6.6.0-rc1-00004-g6faca50f629f #1 Tainted: G                 N

I don't have this exact tree, but on 6.6-rc1,

> [    9.810327] ------------------------------------------------------
> [    9.810406] ip/357 is trying to acquire lock:
> [    9.810501] 83af6c40 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: __flush_work+0x40/0x550
> [    9.811052] 
> [    9.811052] but task is already holding lock:
> [    9.811133] 81639924 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg+0x124/0x514
> [    9.811264] 
> [    9.811264] which lock already depends on the new lock.
> [    9.811264] 
> [    9.811361] 
> [    9.811361] the existing dependency chain (in reverse order) is:
> [    9.811466] 
> [    9.811466] -> #1 (rtnl_mutex){+.+.}-{3:3}:
> [    9.811616]        lock_acquire+0xfc/0x368
> [    9.811717]        __mutex_lock+0x90/0xf00
> [    9.811782]        mutex_lock_nested+0x24/0x2c
> [    9.811845]        ftgmac100_reset+0x1c/0x1dc


This does indeed take the RTNL:

static void ftgmac100_reset(struct ftgmac100 *priv)
{
        struct net_device *netdev = priv->netdev;
        int err;

        netdev_dbg(netdev, "Resetting NIC...\n");

        /* Lock the world */
        rtnl_lock();

and is called from

> [    9.811907]        ftgmac100_adjust_link+0xc0/0x13c
> [    9.811972]        phy_link_change+0x30/0x5c
> [    9.812035]        phy_check_link_status+0x9c/0x11c
> [    9.812100]        phy_state_machine+0x1c0/0x2c0

this work (phy_state_machine is the function), which

> [    9.812405] -> #0 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}:
> [    9.812531]        check_prev_add+0x128/0x15ec
> [    9.812594]        __lock_acquire+0x16ec/0x20cc
> [    9.812656]        lock_acquire+0xfc/0x368
> [    9.812712]        __flush_work+0x70/0x550
> [    9.812769]        __cancel_work_timer+0x1e4/0x264
> [    9.812833]        phy_stop+0x78/0x128

is cancelled by phy_stop() in phy_stop_machine():

void phy_stop_machine(struct phy_device *phydev)
{
        cancel_delayed_work_sync(&phydev->state_queue);

but of course that's called by the driver under RTNL:

> [    9.812889]        ftgmac100_stop+0x5c/0xac
> [    9.812949]        __dev_close_many+0xb8/0x140

(__dev_close_many requires RTNL)


So you have a potential deadlock in this driver. Yes, workqueue items
and RTNL are basically incompatible. Don't do that. Now this bug was
_probably_ added by commit 1baf2e50e48f ("drivers/net/ftgmac100: fix
DHCP potential failure with systemd") which added a call to
ftgmac100_reset() in ftgmac100_adjust_link() which is the thing called
from the PHY state machine in the first place.

Should that be reverted? I don't know ... maybe it can be fixed
differently.


But anyway ... as far as lockdep/workqueue stuff is concerned I'd
definitely call it a win rather than a bug! Yay for making lockdep
useful - it found a deadlock situation for you! :-) No need to blame
lockdep for that :P

johannes

Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

[Guenter Roeck]

On 9/13/23 07:41, Johannes Berg wrote:
> Hi Guenter,
> 
>> This patch results in the attached lockdep splat when running the
>> ast2600-evb emulation in qemu with aspeed_g5_defconfig and lock debugging
>> enabled. Reverting this patch fixes the problem.
> 
> Umm ... That's only true if you think the problem is the lockdep splat,
> rather than the actual potential deadlock?!
> 

It was hard for me to say because the workqueue lock doesn't exist
in the first place if lockdep debugging is not enabled.

>> [    9.809960] ======================================================
>> [    9.810053] WARNING: possible circular locking dependency detected
>> [    9.810196] 6.6.0-rc1-00004-g6faca50f629f #1 Tainted: G                 N
> 
> I don't have this exact tree, but on 6.6-rc1,
> 

Meh, I just included a couple of bug fixes not yet available in 6.6-rc1.

>> [    9.810327] ------------------------------------------------------
>> [    9.810406] ip/357 is trying to acquire lock:
>> [    9.810501] 83af6c40 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: __flush_work+0x40/0x550
>> [    9.811052]
>> [    9.811052] but task is already holding lock:
>> [    9.811133] 81639924 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg+0x124/0x514
>> [    9.811264]
>> [    9.811264] which lock already depends on the new lock.
>> [    9.811264]
>> [    9.811361]
>> [    9.811361] the existing dependency chain (in reverse order) is:
>> [    9.811466]
>> [    9.811466] -> #1 (rtnl_mutex){+.+.}-{3:3}:
>> [    9.811616]        lock_acquire+0xfc/0x368
>> [    9.811717]        __mutex_lock+0x90/0xf00
>> [    9.811782]        mutex_lock_nested+0x24/0x2c
>> [    9.811845]        ftgmac100_reset+0x1c/0x1dc
> 
> 
> This does indeed take the RTNL:
> 
> static void ftgmac100_reset(struct ftgmac100 *priv)
> {
>          struct net_device *netdev = priv->netdev;
>          int err;
> 
>          netdev_dbg(netdev, "Resetting NIC...\n");
> 
>          /* Lock the world */
>          rtnl_lock();
> 
> and is called from
> 
>> [    9.811907]        ftgmac100_adjust_link+0xc0/0x13c
>> [    9.811972]        phy_link_change+0x30/0x5c
>> [    9.812035]        phy_check_link_status+0x9c/0x11c
>> [    9.812100]        phy_state_machine+0x1c0/0x2c0
> 
> this work (phy_state_machine is the function), which
> 
>> [    9.812405] -> #0 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}:
>> [    9.812531]        check_prev_add+0x128/0x15ec
>> [    9.812594]        __lock_acquire+0x16ec/0x20cc
>> [    9.812656]        lock_acquire+0xfc/0x368
>> [    9.812712]        __flush_work+0x70/0x550
>> [    9.812769]        __cancel_work_timer+0x1e4/0x264
>> [    9.812833]        phy_stop+0x78/0x128
> 
> is cancelled by phy_stop() in phy_stop_machine():
> 
> void phy_stop_machine(struct phy_device *phydev)
> {
>          cancel_delayed_work_sync(&phydev->state_queue);
> 
> but of course that's called by the driver under RTNL:
> 
>> [    9.812889]        ftgmac100_stop+0x5c/0xac
>> [    9.812949]        __dev_close_many+0xb8/0x140
> 
> (__dev_close_many requires RTNL)
> 
> 
> So you have a potential deadlock in this driver. Yes, workqueue items
> and RTNL are basically incompatible. Don't do that. Now this bug was
> _probably_ added by commit 1baf2e50e48f ("drivers/net/ftgmac100: fix
> DHCP potential failure with systemd") which added a call to
> ftgmac100_reset() in ftgmac100_adjust_link() which is the thing called
> from the PHY state machine in the first place.
> 
> Should that be reverted? I don't know ... maybe it can be fixed
> differently.
> 
> 
> But anyway ... as far as lockdep/workqueue stuff is concerned I'd
> definitely call it a win rather than a bug! Yay for making lockdep
> useful - it found a deadlock situation for you! :-) No need to blame
> lockdep for that :P
> 

So you are saying that anything running in a workqueue must not
acquire rtnl_lock because cancel_[delayed_]work_sync() may be called
under rtnl_lock.

Fair point, but is that documented somewhere ? If not, how is anyone
supposed to know ? If it is not documented, I might we well argue that
cancel_[delayed_]work_sync() should not be called with rtnl_lock held
because some worker might hold that lock.

FWIW, it would be nice if the lockdep code would generate some other
message in this situation. Complaining about a deadlock involving a
lock that doesn't exist if lock debugging isn't enabled is not really
helpful and, yes, may result in reporters to falsely assume that this
lock is responsible for the potential deadlock.

Reverting 1baf2e50e48f does fix the problem as well.

Thanks,
Guenter

Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 5415 bytes --]

On 9/13/23 08:59, Guenter Roeck wrote:
> On 9/13/23 07:41, Johannes Berg wrote:
>> Hi Guenter,
>>
>>> This patch results in the attached lockdep splat when running the
>>> ast2600-evb emulation in qemu with aspeed_g5_defconfig and lock 
>>> debugging
>>> enabled. Reverting this patch fixes the problem.
>>
>> Umm ... That's only true if you think the problem is the lockdep splat,
>> rather than the actual potential deadlock?!
>>
> 
> It was hard for me to say because the workqueue lock doesn't exist
> in the first place if lockdep debugging is not enabled.
> 
>>> [    9.809960] ======================================================
>>> [    9.810053] WARNING: possible circular locking dependency detected
>>> [    9.810196] 6.6.0-rc1-00004-g6faca50f629f #1 Tainted: 
>>> G                 N
>>
>> I don't have this exact tree, but on 6.6-rc1,
>>
> 
> Meh, I just included a couple of bug fixes not yet available in 6.6-rc1.
> 
>>> [    9.810327] ------------------------------------------------------
>>> [    9.810406] ip/357 is trying to acquire lock:
>>> [    9.810501] 83af6c40 
>>> ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: 
>>> __flush_work+0x40/0x550
>>> [    9.811052]
>>> [    9.811052] but task is already holding lock:
>>> [    9.811133] 81639924 (rtnl_mutex){+.+.}-{3:3}, at: 
>>> rtnetlink_rcv_msg+0x124/0x514
>>> [    9.811264]
>>> [    9.811264] which lock already depends on the new lock.
>>> [    9.811264]
>>> [    9.811361]
>>> [    9.811361] the existing dependency chain (in reverse order) is:
>>> [    9.811466]
>>> [    9.811466] -> #1 (rtnl_mutex){+.+.}-{3:3}:
>>> [    9.811616]        lock_acquire+0xfc/0x368
>>> [    9.811717]        __mutex_lock+0x90/0xf00
>>> [    9.811782]        mutex_lock_nested+0x24/0x2c
>>> [    9.811845]        ftgmac100_reset+0x1c/0x1dc
>>
>>
>> This does indeed take the RTNL:
>>
>> static void ftgmac100_reset(struct ftgmac100 *priv)
>> {
>>          struct net_device *netdev = priv->netdev;
>>          int err;
>>
>>          netdev_dbg(netdev, "Resetting NIC...\n");
>>
>>          /* Lock the world */
>>          rtnl_lock();
>>
>> and is called from
>>
>>> [    9.811907]        ftgmac100_adjust_link+0xc0/0x13c
>>> [    9.811972]        phy_link_change+0x30/0x5c
>>> [    9.812035]        phy_check_link_status+0x9c/0x11c
>>> [    9.812100]        phy_state_machine+0x1c0/0x2c0
>>
>> this work (phy_state_machine is the function), which
>>
>>> [    9.812405] -> #0 
>>> ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}:
>>> [    9.812531]        check_prev_add+0x128/0x15ec
>>> [    9.812594]        __lock_acquire+0x16ec/0x20cc
>>> [    9.812656]        lock_acquire+0xfc/0x368
>>> [    9.812712]        __flush_work+0x70/0x550
>>> [    9.812769]        __cancel_work_timer+0x1e4/0x264
>>> [    9.812833]        phy_stop+0x78/0x128
>>
>> is cancelled by phy_stop() in phy_stop_machine():
>>
>> void phy_stop_machine(struct phy_device *phydev)
>> {
>>          cancel_delayed_work_sync(&phydev->state_queue);
>>
>> but of course that's called by the driver under RTNL:
>>
>>> [    9.812889]        ftgmac100_stop+0x5c/0xac
>>> [    9.812949]        __dev_close_many+0xb8/0x140
>>
>> (__dev_close_many requires RTNL)
>>
>>
>> So you have a potential deadlock in this driver. Yes, workqueue items
>> and RTNL are basically incompatible. Don't do that. Now this bug was
>> _probably_ added by commit 1baf2e50e48f ("drivers/net/ftgmac100: fix
>> DHCP potential failure with systemd") which added a call to
>> ftgmac100_reset() in ftgmac100_adjust_link() which is the thing called
>> from the PHY state machine in the first place.
>>
>> Should that be reverted? I don't know ... maybe it can be fixed
>> differently.
>>
>>
>> But anyway ... as far as lockdep/workqueue stuff is concerned I'd
>> definitely call it a win rather than a bug! Yay for making lockdep
>> useful - it found a deadlock situation for you! :-) No need to blame
>> lockdep for that :P
>>
> 
> So you are saying that anything running in a workqueue must not
> acquire rtnl_lock because cancel_[delayed_]work_sync() may be called
> under rtnl_lock.
> 
> Fair point, but is that documented somewhere ? If not, how is anyone
> supposed to know ? If it is not documented, I might we well argue that
> cancel_[delayed_]work_sync() should not be called with rtnl_lock held
> because some worker might hold that lock.
> 
> FWIW, it would be nice if the lockdep code would generate some other
> message in this situation. Complaining about a deadlock involving a
> lock that doesn't exist if lock debugging isn't enabled is not really
> helpful and, yes, may result in reporters to falsely assume that this
> lock is responsible for the potential deadlock.
> 
> Reverting 1baf2e50e48f does fix the problem as well.

I would refrain from reverting without giving a fighting chance to the 
author to address it. It seems a bit strange that we do this locking 
dance while it might have been simpler to introduce a 
ftgmac100_reset_unlocked() and ftgmac100_reset() and call both at the 
intended places, something like the completely untested patch attached 
maybe?
-- 
Florian

[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 1905 bytes --]

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index a03879a27b04..253bf6d66280 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1299,24 +1299,17 @@ static int ftgmac100_init_all(struct ftgmac100 *priv, bool ignore_alloc_err)
 	return err;
 }
 
-static void ftgmac100_reset(struct ftgmac100 *priv)
+static void ftgmac100_reset_unlocked(struct ftgmac100 *priv)
 {
 	struct net_device *netdev = priv->netdev;
 	int err;
 
 	netdev_dbg(netdev, "Resetting NIC...\n");
 
-	/* Lock the world */
-	rtnl_lock();
-	if (netdev->phydev)
-		mutex_lock(&netdev->phydev->lock);
-	if (priv->mii_bus)
-		mutex_lock(&priv->mii_bus->mdio_lock);
-
 
 	/* Check if the interface is still up */
 	if (!netif_running(netdev))
-		goto bail;
+		return;
 
 	/* Stop the network stack */
 	netif_trans_update(netdev);
@@ -1338,7 +1331,19 @@ static void ftgmac100_reset(struct ftgmac100 *priv)
 	ftgmac100_init_all(priv, true);
 
 	netdev_dbg(netdev, "Reset done !\n");
- bail:
+}
+
+static void ftgmac100_reset(struct ftgmac100 *priv)
+{
+	struct net_device *netdev = priv->netdev;
+
+	rtnl_lock();
+	/* Lock the world */
+	if (netdev->phydev)
+		mutex_lock(&netdev->phydev->lock);
+	if (priv->mii_bus)
+		mutex_lock(&priv->mii_bus->mdio_lock);
+	ftgmac100_reset_unlocked(priv);
 	if (priv->mii_bus)
 		mutex_unlock(&priv->mii_bus->mdio_lock);
 	if (netdev->phydev)
@@ -1405,14 +1410,7 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 	/* Release phy lock to allow ftgmac100_reset to aquire it, keeping lock
 	 * order consistent to prevent dead lock.
 	 */
-	if (netdev->phydev)
-		mutex_unlock(&netdev->phydev->lock);
-
-	ftgmac100_reset(priv);
-
-	if (netdev->phydev)
-		mutex_lock(&netdev->phydev->lock);
-
+	ftgmac100_reset_unlocked(priv);
 }
 
 static int ftgmac100_mii_probe(struct net_device *netdev)

Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

[Guenter Roeck]

On 9/13/23 10:25, Florian Fainelli wrote:
> On 9/13/23 08:59, Guenter Roeck wrote:
>> On 9/13/23 07:41, Johannes Berg wrote:
>>> Hi Guenter,
>>>
>>>> This patch results in the attached lockdep splat when running the
>>>> ast2600-evb emulation in qemu with aspeed_g5_defconfig and lock debugging
>>>> enabled. Reverting this patch fixes the problem.
>>>
>>> Umm ... That's only true if you think the problem is the lockdep splat,
>>> rather than the actual potential deadlock?!
>>>
>>
>> It was hard for me to say because the workqueue lock doesn't exist
>> in the first place if lockdep debugging is not enabled.
>>
>>>> [    9.809960] ======================================================
>>>> [    9.810053] WARNING: possible circular locking dependency detected
>>>> [    9.810196] 6.6.0-rc1-00004-g6faca50f629f #1 Tainted: G                 N
>>>
>>> I don't have this exact tree, but on 6.6-rc1,
>>>
>>
>> Meh, I just included a couple of bug fixes not yet available in 6.6-rc1.
>>
>>>> [    9.810327] ------------------------------------------------------
>>>> [    9.810406] ip/357 is trying to acquire lock:
>>>> [    9.810501] 83af6c40 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: __flush_work+0x40/0x550
>>>> [    9.811052]
>>>> [    9.811052] but task is already holding lock:
>>>> [    9.811133] 81639924 (rtnl_mutex){+.+.}-{3:3}, at: rtnetlink_rcv_msg+0x124/0x514
>>>> [    9.811264]
>>>> [    9.811264] which lock already depends on the new lock.
>>>> [    9.811264]
>>>> [    9.811361]
>>>> [    9.811361] the existing dependency chain (in reverse order) is:
>>>> [    9.811466]
>>>> [    9.811466] -> #1 (rtnl_mutex){+.+.}-{3:3}:
>>>> [    9.811616]        lock_acquire+0xfc/0x368
>>>> [    9.811717]        __mutex_lock+0x90/0xf00
>>>> [    9.811782]        mutex_lock_nested+0x24/0x2c
>>>> [    9.811845]        ftgmac100_reset+0x1c/0x1dc
>>>
>>>
>>> This does indeed take the RTNL:
>>>
>>> static void ftgmac100_reset(struct ftgmac100 *priv)
>>> {
>>>          struct net_device *netdev = priv->netdev;
>>>          int err;
>>>
>>>          netdev_dbg(netdev, "Resetting NIC...\n");
>>>
>>>          /* Lock the world */
>>>          rtnl_lock();
>>>
>>> and is called from
>>>
>>>> [    9.811907]        ftgmac100_adjust_link+0xc0/0x13c
>>>> [    9.811972]        phy_link_change+0x30/0x5c
>>>> [    9.812035]        phy_check_link_status+0x9c/0x11c
>>>> [    9.812100]        phy_state_machine+0x1c0/0x2c0
>>>
>>> this work (phy_state_machine is the function), which
>>>
>>>> [    9.812405] -> #0 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}:
>>>> [    9.812531]        check_prev_add+0x128/0x15ec
>>>> [    9.812594]        __lock_acquire+0x16ec/0x20cc
>>>> [    9.812656]        lock_acquire+0xfc/0x368
>>>> [    9.812712]        __flush_work+0x70/0x550
>>>> [    9.812769]        __cancel_work_timer+0x1e4/0x264
>>>> [    9.812833]        phy_stop+0x78/0x128
>>>
>>> is cancelled by phy_stop() in phy_stop_machine():
>>>
>>> void phy_stop_machine(struct phy_device *phydev)
>>> {
>>>          cancel_delayed_work_sync(&phydev->state_queue);
>>>
>>> but of course that's called by the driver under RTNL:
>>>
>>>> [    9.812889]        ftgmac100_stop+0x5c/0xac
>>>> [    9.812949]        __dev_close_many+0xb8/0x140
>>>
>>> (__dev_close_many requires RTNL)
>>>
>>>
>>> So you have a potential deadlock in this driver. Yes, workqueue items
>>> and RTNL are basically incompatible. Don't do that. Now this bug was
>>> _probably_ added by commit 1baf2e50e48f ("drivers/net/ftgmac100: fix
>>> DHCP potential failure with systemd") which added a call to
>>> ftgmac100_reset() in ftgmac100_adjust_link() which is the thing called
>>> from the PHY state machine in the first place.
>>>
>>> Should that be reverted? I don't know ... maybe it can be fixed
>>> differently.
>>>
>>>
>>> But anyway ... as far as lockdep/workqueue stuff is concerned I'd
>>> definitely call it a win rather than a bug! Yay for making lockdep
>>> useful - it found a deadlock situation for you! :-) No need to blame
>>> lockdep for that :P
>>>
>>
>> So you are saying that anything running in a workqueue must not
>> acquire rtnl_lock because cancel_[delayed_]work_sync() may be called
>> under rtnl_lock.
>>
>> Fair point, but is that documented somewhere ? If not, how is anyone
>> supposed to know ? If it is not documented, I might we well argue that
>> cancel_[delayed_]work_sync() should not be called with rtnl_lock held
>> because some worker might hold that lock.
>>
>> FWIW, it would be nice if the lockdep code would generate some other
>> message in this situation. Complaining about a deadlock involving a
>> lock that doesn't exist if lock debugging isn't enabled is not really
>> helpful and, yes, may result in reporters to falsely assume that this
>> lock is responsible for the potential deadlock.
>>
>> Reverting 1baf2e50e48f does fix the problem as well.
> 
> I would refrain from reverting without giving a fighting chance to the author to address it. It seems a bit strange that we do this locking dance while it might have been simpler to introduce a ftgmac100_reset_unlocked() and ftgmac100_reset() and call both at the intended places, something like the completely untested patch attached maybe?


I agree. As it turns out, there are lots of "workqueue items" in the
kernel calling rtnl_lock(), and at least some of them are canceled
with cancel_delayed_work_sync(). So there has to be some additional
qualifying factor, such as that it is safe to acquire rtnl_lock() in a
worker as long as its cancel function is not called with the lock held.

I don't know if the attached patch would work because I don't know the
impact of executing ftgmac100_reset() without holding the locks.
Also, after all, the problem isnt't the lock itself, but the fact that
cancel_delayed_work_sync() may be called on this specific worker with
rtnl_lock held.

Guenter

Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

[Johannes Berg]

On Wed, 2023-09-13 at 10:25 -0700, Florian Fainelli wrote:
> 
> I would refrain from reverting without giving a fighting chance to the 
> author to address it. It seems a bit strange that we do this locking 
> dance while it might have been simpler to introduce a 
> ftgmac100_reset_unlocked() and ftgmac100_reset() and call both at the 
> intended places, something like the completely untested patch attached 
> maybe?

Not sure that's right - it probably wants RTNL for some reason, but with
this patch you don't hold it when coming from ftgmac100_adjust_link()?
(If it did, it'd have deadlocked getting here after all, since it
acquires it)

Not sure why it needs RTNL though, that doesn't seem so bad, and holds
some internal locks. Or maybe it doesn't really, only if there's a
phydev and/or mii_bus, so maybe it needs a driver lock? Well, there's a
note about the reset task, that might be it?

static int ftgmac100_stop(struct net_device *netdev)
{
        struct ftgmac100 *priv = netdev_priv(netdev);

        /* Note about the reset task: We are called with the rtnl lock
         * held, so we are synchronized against the core of the reset
         * task. We must not try to synchronously cancel it otherwise
         * we can deadlock. But since it will test for netif_running()
         * which has already been cleared by the net core, we don't
         * anything special to do.
         */



But it really kind of feels the locking model in this driver is a bit
off.

johannes

Re: [PATCH v3] workqueue: don't skip lockdep work dependency in cancel_work_sync()

[Johannes Berg]

On Wed, 2023-09-13 at 08:59 -0700, Guenter Roeck wrote:
> 
> So you are saying that anything running in a workqueue must not
> acquire rtnl_lock because cancel_[delayed_]work_sync() may be called
> under rtnl_lock.

No no, sorry if I wasn't clear. I mean this particular function / work
struct cannot acquire the RTNL because the cancel _for it_ is called
under RTNL.

It used to be that this was also tied to the entire workqueue, but this
is no longer true due to the way workqueues work these days.


> FWIW, it would be nice if the lockdep code would generate some other
> message in this situation. Complaining about a deadlock involving a
> lock that doesn't exist if lock debugging isn't enabled is not really
> helpful and, yes, may result in reporters to falsely assume that this
> lock is responsible for the potential deadlock.

Well, I don't know of any way to tell lockdep that, but I guess ideas
welcome? I mean, I'm not even sure what else it would tell you, other
than that you have a deadlock?

I mean, OK, I guess it's fair - it says "acquire lock" when it says

[    9.810406] ip/357 is trying to acquire lock:
[    9.810501] 83af6c40 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: __flush_work+0x40/0x550

and it's not really a lock, but I'm not even sure how to phrase it
better? Note the scenario may be more complex than here.

I mean, perhaps we could add an optional message somehow and it could
say

"ip/357 is waiting for the work:"

but then we'd also have to update the scenario message to something like

[    9.813938]        CPU0                    CPU1
[    9.813999]        ----                    ----
[    9.814062]   lock(rtnl_mutex);
[    9.814139]                                run((work_completion)(&(&dev->state_queue)->work));
[    9.814258]                                lock(rtnl_mutex);
[    9.814354]   wait((work_completion)(&(&dev->state_queue)->work));


which is really hard to do because how should lockdep know that the two
ways of "acquiring the lock" are actually different, and which one is
which? I'm not even convinced it could really do that.

In any case, I'd rather have a bug report from this than not, even if
it's not trivial to read.


... and here I thought we went through all of this 15+ years ago when I
added it in commit 4e6045f13478 ("workqueue: debug flushing deadlocks
with lockdep"), at which time the situation was actually worse because
you had to not only pay attention to the work struct, but also the
entire workqueue - which is today only true for ordered workqueues... Oh
well :)

johannes