Re: [PATCH net] net: dm9000: Fix a recursive lockup in the TX timeout path

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In drivers/net/ethernet/davicom/dm9000.c, dm9000_phy_write() now
  decides whether to take and release both db->addr_lock and db->lock by…
- [Medium] in_timeout, in_suspend and wake_supported are adjacent 1-bit
  bitfields in the same unsigned int of struct board_info (lines…

--

net: dm9000: Fix a recursive lockup in the TX timeout path

This commit fixes a deadlock on DM9000B. There, dm9000_timeout() holds
db->lock while dm9000_init_dm9000() calls dm9000_phy_write(), which takes
db->lock again. dm9000_phy_write() already skipped db->addr_lock when
db->in_timeout was set, and it now skips db->lock in that case too.

> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index b87eaf0c250ce..328043056ec48 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -325,10 +325,10 @@ dm9000_phy_write(struct net_device *dev,
>  	unsigned long reg_save;
>  
>  	dm9000_dbg(db, 5, "phy_write[%02x] = %04x\n", reg, value);
> -	if (!db->in_timeout)
> +	if (!db->in_timeout) {
>  		mutex_lock(&db->addr_lock);
> -
> -	spin_lock_irqsave(&db->lock, flags);
> +		spin_lock_irqsave(&db->lock, flags);
> +	}
>  
>  	/* Save previous register address */
>  	reg_save = readb(db->io_addr);
> @@ -344,11 +344,14 @@ dm9000_phy_write(struct net_device *dev,
>  	iow(db, DM9000_EPCR, EPCR_EPOS | EPCR_ERPRW);
>  
>  	writeb(reg_save, db->io_addr);
> -	spin_unlock_irqrestore(&db->lock, flags);
> +	if (!db->in_timeout)
> +		spin_unlock_irqrestore(&db->lock, flags);
>  
>  	dm9000_msleep(db, 1);		/* Wait write complete */
>  
> -	spin_lock_irqsave(&db->lock, flags);
> +	if (!db->in_timeout)
> +		spin_lock_irqsave(&db->lock, flags);

[Severity: High]
Can in_timeout change between these checks when dm9000_phy_write() is
called from somewhere other than dm9000_timeout()?

in_timeout is a device-wide flag. dm9000_timeout() sets and clears it, and
it runs from the dev_watchdog timer with only tx_global_lock held.

dm9000_phy_write() is also the mii.mdio_write callback. It is reached under
rtnl through these paths:

  dm9000_nway_reset()->mii_nway_restart()
  dm9000_set_link_ksettings()->mii_ethtool_set_link_ksettings()
  dm9000_ioctl()->generic_mii_ioctl() (SIOCSMIIREG)

None of these paths are serialized against the watchdog. So
in_timeout == 1 only means that some CPU is inside dm9000_timeout(). It
does not mean that this caller holds db->lock.

The flag is now read four times without a lock, and each read decides
whether a lock is taken or released. Before this patch the relock after
dm9000_msleep() was unconditional. A caller would spin until
dm9000_timeout() finished, so the final check always ran under db->lock,
where in_timeout can only be 0.

Consider this sequence on DM9000B:

CPU0 (ethtool -r)                  CPU1 (dev_watchdog)
dm9000_phy_write()
  in_timeout == 0
  mutex_lock(&db->addr_lock)
  spin_lock_irqsave(&db->lock)
  iow(db, DM9000_EPCR, ...)
  spin_unlock_irqrestore(&db->lock)
  dm9000_msleep(db, 1)
                                   dm9000_timeout()
                                     spin_lock_irqsave(&db->lock)
                                     db->in_timeout = 1
                                     dm9000_init_dm9000()
                                       dm9000_phy_write() x2
                                         mdelay(1) each
  in_timeout == 1, relock skipped
  readb(db->io_addr), iow() ...    iow() ...
  in_timeout == 1, unlocks skipped

CPU0's index/data accesses then interleave with CPU1's iow() pairs, and
db->addr_lock is never released. Would later phy_read, EEPROM and
ethtool/MII callers then hang, often with rtnl held?

If CPU1 clears the flag before CPU0 reaches the final check, CPU0 instead
calls spin_unlock_irqrestore() on a db->lock it doesn't hold.

The opposite case also seems possible. If CPU0 enters while
in_timeout == 1, it takes neither lock and flags is never initialized. If
the flag then drops to 0 before the first unlock check, CPU0 runs
spin_unlock_irqrestore(&db->lock, flags) with garbage flags. Later it runs
mutex_unlock() on an addr_lock it never took.

Would it be more robust to stop inferring lock ownership from in_timeout?
For example, the timeout path in dm9000_init_dm9000() could call an
unlocked __dm9000_phy_write() helper (or pass a locked argument), and
normal MDIO callers would always take both locks.

> +
>  	reg_save = readb(db->io_addr);
>  
>  	iow(db, DM9000_EPCR, 0x0);	/* Clear phyxcer write command */
> @@ -356,9 +359,10 @@ dm9000_phy_write(struct net_device *dev,
>  	/* restore the previous address */
>  	writeb(reg_save, db->io_addr);
>  
> -	spin_unlock_irqrestore(&db->lock, flags);
> -	if (!db->in_timeout)
> +	if (!db->in_timeout) {
> +		spin_unlock_irqrestore(&db->lock, flags);
>  		mutex_unlock(&db->addr_lock);
> +	}
>  }

[Severity: Medium]
With this change, in_timeout effectively means "the caller already holds
db->lock". Could a stale in_timeout value leave every later
dm9000_phy_write() running without db->lock or addr_lock?

in_timeout shares a word with in_suspend and wake_supported in struct
board_info:

	unsigned int	in_timeout:1;
	unsigned int	in_suspend:1;
	unsigned int	wake_supported:1;

dm9000_timeout() writes in_timeout under db->lock. dm9000_drv_suspend() and
dm9000_drv_resume() write in_suspend without any lock.

In dm9000_drv_suspend() the store happens before netif_device_detach(), so
the watchdog can still be running dm9000_timeout() on another CPU:

	db->in_suspend = 1;

	if (!netif_running(ndev))
		return 0;

	netif_device_detach(ndev);

dm9000_drv_resume() likewise clears in_suspend after netif_device_attach()
has already re-enabled the watchdog.

Suppose the suspend read-modify-write loads the word while in_timeout is 1,
and stores it back after dm9000_timeout() has cleared the flag. in_timeout
then stays at 1 until the next TX timeout.

The lost update itself predates this patch, but before this patch it only
caused addr_lock to be skipped. Now these later dm9000_phy_write() callers
would skip both locks:

  resume, via dm9000_init_dm9000() on DM9000B
  ethtool and MII ioctls
  dm9000_shutdown()

Their index/data register accesses would then race with the xmit and IRQ
paths.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923084055.65091-1-ginger.jzllee%40gmail.com