linux-kernel - Re: [PATCH 6.1 000/243] 6.1.91-rc2 review

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d52e7e4a-2b60-4fdf-9006-12528a91dabf@broadcom.com>
Date: Wed, 15 May 2024 09:54:49 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
 Mark Brown <broonie@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, stable@...r.kernel.org,
 patches@...ts.linux.dev, linux-kernel@...r.kernel.org,
 akpm@...ux-foundation.org, linux@...ck-us.net, shuah@...nel.org,
 patches@...nelci.org, lkft-triage@...ts.linaro.org, pavel@...x.de,
 jonathanh@...dia.com, sudipm.mukherjee@...il.com, srw@...dewatkins.net,
 rwarsow@....de, conor@...nel.org, allen.lkml@...il.com,
 Doug Berger <opendmb@...il.com>
Subject: Re: [PATCH 6.1 000/243] 6.1.91-rc2 review

On 5/15/24 09:40, Florian Fainelli wrote:
> On 5/15/24 09:20, Linus Torvalds wrote:
>> On Wed, 15 May 2024 at 09:17, Mark Brown <broonie@...nel.org> wrote:
>>>
>>>      A bisect claims that "net: bcmgenet:
>>> synchronize EXT_RGMII_OOB_CTRL access" is the first commit that breaks,
>>> I'm not seeing issues with other stables.
>>
>> That's d85cf67a3396 ("net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL
>> access") upstream. Is upstream ok?
> 
> Yes, upstream is OK.
> 
> Exact same issue on 6.1 not just on Raspberry Pi chips but also on 
> ARCH_BRCMSTB systems as well that also make use of the GENET driver.
> 
> Doug and I will take a look and provide an updated set of fixes, 
> meanwhile, I would recommend dropping all of Doug's patches for now 
> until we can post a revised series and/or missing dependencies.

OK, I think I see the problem. The upstream patch was applied to the 
wrong context. In upstream, the critical section is added to 
bcmgenet_mii_config() which did not run with the phy_device::mutex_lock 
held before, and that was the race that we are trying to close. In 
linux-stable-rc/linux-6.1.y however the context somehow resolved to 
applying the critical section to the bcmgenet_mac_config() section where 
we are already running with phy_device::lock being held by virtue of the 
PHY library running our bcmgenet_mac_config() callback.

The corrected incremental diff would look like this:

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c 
b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 94e0e858266e..46252d96b90e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -71,12 +71,10 @@ static void bcmgenet_mac_config(struct net_device *dev)
          * transmit -- 25MHz(100Mbps) or 125MHz(1Gbps).
          * Receive clock is provided by the PHY.
          */
-       mutex_lock(&phydev->lock);
         reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
         reg &= ~OOB_DISABLE;
         reg |= RGMII_LINK;
         bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
-       mutex_unlock(&phydev->lock);

         spin_lock_bh(&priv->reg_lock);
         reg = bcmgenet_umac_readl(priv, UMAC_CMD);
@@ -271,6 +269,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool 
init)
          * block for the interface to work
          */
         if (priv->ext_phy) {
+               mutex_lock(&phydev->lock);
                 reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL);
                 reg &= ~ID_MODE_DIS;
                 reg |= id_mode_dis;
@@ -279,6 +278,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool 
init)
                 else
                         reg |= RGMII_MODE_EN;
                 bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL);
+               mutex_unlock(&phydev->lock);
         }

         if (init)

We could, and should consider taking 
696450c05181559a35d4d5bee55c465b1ac6fe2e ("net: bcmgenet: Clear 
RGMII_LINK upon link down") as a prerequisite as well to provide the 
right context while applying  d85cf67a339685beae1d0aee27b7f61da95455be 
("net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access")

Mark, can you try that, too?
-- 
Florian


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)