* [PATCH 0/3] net: bcmgenet: protect contended accesses
@ 2024-04-25 22:10 Doug Berger
2024-04-25 22:10 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Doug Berger @ 2024-04-25 22:10 UTC (permalink / raw)
To: netdev; +Cc: Doug Berger
Some registers may be modified by parallel execution contexts and
require protections to prevent corruption.
A review of the driver revealed the need for these additional
protections.
Doug Berger (3):
net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access
net: bcmgenet: synchronize use of bcmgenet_set_rx_mode()
net: bcmgenet: synchronize UMAC_CMD access
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 16 ++++++++++++++--
drivers/net/ethernet/broadcom/genet/bcmgenet.h | 4 +++-
.../net/ethernet/broadcom/genet/bcmgenet_wol.c | 8 +++++++-
drivers/net/ethernet/broadcom/genet/bcmmii.c | 6 +++++-
4 files changed, 29 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access 2024-04-25 22:10 [PATCH 0/3] net: bcmgenet: protect contended accesses Doug Berger @ 2024-04-25 22:10 ` Doug Berger 2024-04-27 14:16 ` Simon Horman 2024-04-25 22:10 ` [PATCH 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() Doug Berger 2024-04-25 22:10 ` [PATCH 3/3] net: bcmgenet: synchronize UMAC_CMD access Doug Berger 2 siblings, 1 reply; 8+ messages in thread From: Doug Berger @ 2024-04-25 22:10 UTC (permalink / raw) To: netdev; +Cc: Doug Berger, stable The EXT_RGMII_OOB_CTRL register can be written from different contexts. It is predominantly written from the adjust_link handler which is synchronized by the phydev->lock, but can also be written from a different context when configuring the mii in bcmgenet_mii_config(). The chances of contention are quite low, but it is conceivable that adjust_link could occur during resume when WoL is enabled so use the phydev->lock synchronizer in bcmgenet_mii_config() to be sure. Fixes: afe3f907d20f ("net: bcmgenet: power on MII block for all MII modes") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger <opendmb@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 9ada89355747..86a4aa72b3d4 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -2,7 +2,7 @@ /* * Broadcom GENET MDIO routines * - * Copyright (c) 2014-2017 Broadcom + * Copyright (c) 2014-2024 Broadcom */ #include <linux/acpi.h> @@ -275,6 +275,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) * block for the interface to work, unconditionally clear the * Out-of-band disable since we do not need it. */ + mutex_lock(&phydev->lock); reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL); reg &= ~OOB_DISABLE; if (priv->ext_phy) { @@ -286,6 +287,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) reg |= RGMII_MODE_EN; } bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL); + mutex_unlock(&phydev->lock); if (init) dev_info(kdev, "configuring instance for %s\n", phy_name); -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access 2024-04-25 22:10 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger @ 2024-04-27 14:16 ` Simon Horman 0 siblings, 0 replies; 8+ messages in thread From: Simon Horman @ 2024-04-27 14:16 UTC (permalink / raw) To: Doug Berger; +Cc: netdev, stable On Thu, Apr 25, 2024 at 03:10:05PM -0700, Doug Berger wrote: > The EXT_RGMII_OOB_CTRL register can be written from different > contexts. It is predominantly written from the adjust_link > handler which is synchronized by the phydev->lock, but can > also be written from a different context when configuring the > mii in bcmgenet_mii_config(). > > The chances of contention are quite low, but it is conceivable > that adjust_link could occur during resume when WoL is enabled > so use the phydev->lock synchronizer in bcmgenet_mii_config() > to be sure. > > Fixes: afe3f907d20f ("net: bcmgenet: power on MII block for all MII modes") > Cc: stable@vger.kernel.org > Signed-off-by: Doug Berger <opendmb@gmail.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() 2024-04-25 22:10 [PATCH 0/3] net: bcmgenet: protect contended accesses Doug Berger 2024-04-25 22:10 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger @ 2024-04-25 22:10 ` Doug Berger 2024-04-27 14:16 ` Simon Horman 2024-04-25 22:10 ` [PATCH 3/3] net: bcmgenet: synchronize UMAC_CMD access Doug Berger 2 siblings, 1 reply; 8+ messages in thread From: Doug Berger @ 2024-04-25 22:10 UTC (permalink / raw) To: netdev; +Cc: Doug Berger, stable The ndo_set_rx_mode function is synchronized with the netif_addr_lock spinlock and BHs disabled. Since this function is also invoked directly from the driver the same synchronization should be applied. Fixes: 72f96347628e ("net: bcmgenet: set Rx mode before starting netif") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger <opendmb@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index b1f84b37032a..5452b7dc6e6a 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -2,7 +2,7 @@ /* * Broadcom GENET (Gigabit Ethernet) controller driver * - * Copyright (c) 2014-2020 Broadcom + * Copyright (c) 2014-2024 Broadcom */ #define pr_fmt(fmt) "bcmgenet: " fmt @@ -3334,7 +3334,9 @@ static void bcmgenet_netif_start(struct net_device *dev) struct bcmgenet_priv *priv = netdev_priv(dev); /* Start the network engine */ + netif_addr_lock_bh(dev); bcmgenet_set_rx_mode(dev); + netif_addr_unlock_bh(dev); bcmgenet_enable_rx_napi(priv); umac_enable_set(priv, CMD_TX_EN | CMD_RX_EN, true); -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() 2024-04-25 22:10 ` [PATCH 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() Doug Berger @ 2024-04-27 14:16 ` Simon Horman 0 siblings, 0 replies; 8+ messages in thread From: Simon Horman @ 2024-04-27 14:16 UTC (permalink / raw) To: Doug Berger; +Cc: netdev, stable On Thu, Apr 25, 2024 at 03:10:06PM -0700, Doug Berger wrote: > The ndo_set_rx_mode function is synchronized with the > netif_addr_lock spinlock and BHs disabled. Since this > function is also invoked directly from the driver the > same synchronization should be applied. > > Fixes: 72f96347628e ("net: bcmgenet: set Rx mode before starting netif") > Cc: stable@vger.kernel.org > Signed-off-by: Doug Berger <opendmb@gmail.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] net: bcmgenet: synchronize UMAC_CMD access 2024-04-25 22:10 [PATCH 0/3] net: bcmgenet: protect contended accesses Doug Berger 2024-04-25 22:10 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger 2024-04-25 22:10 ` [PATCH 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() Doug Berger @ 2024-04-25 22:10 ` Doug Berger 2024-04-27 14:16 ` Simon Horman 2 siblings, 1 reply; 8+ messages in thread From: Doug Berger @ 2024-04-25 22:10 UTC (permalink / raw) To: netdev; +Cc: Doug Berger, stable The UMAC_CMD register is written from different execution contexts and has insufficient synchronization protections to prevent possible corruption. Of particular concern are the acceses from the phy_device delayed work context used by the adjust_link call and the BH context that may be used by the ndo_set_rx_mode call. A spinlock is added to the driver to protect contended register accesses (i.e. reg_lock) and it is used to synchronize accesses to UMAC_CMD. Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger <opendmb@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 12 +++++++++++- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 4 +++- drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 8 +++++++- drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 ++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 5452b7dc6e6a..c7e7dac057a3 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -2467,14 +2467,18 @@ static void umac_enable_set(struct bcmgenet_priv *priv, u32 mask, bool enable) { u32 reg; + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); - if (reg & CMD_SW_RESET) + if (reg & CMD_SW_RESET) { + spin_unlock_bh(&priv->reg_lock); return; + } if (enable) reg |= mask; else reg &= ~mask; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); /* UniMAC stops on a packet boundary, wait for a full-size packet * to be processed @@ -2490,8 +2494,10 @@ static void reset_umac(struct bcmgenet_priv *priv) udelay(10); /* issue soft reset and disable MAC while updating its registers */ + spin_lock_bh(&priv->reg_lock); bcmgenet_umac_writel(priv, CMD_SW_RESET, UMAC_CMD); udelay(2); + spin_unlock_bh(&priv->reg_lock); } static void bcmgenet_intr_disable(struct bcmgenet_priv *priv) @@ -3597,16 +3603,19 @@ static void bcmgenet_set_rx_mode(struct net_device *dev) * 3. The number of filters needed exceeds the number filters * supported by the hardware. */ + spin_lock(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); if ((dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) || (nfilter > MAX_MDF_FILTER)) { reg |= CMD_PROMISC; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock(&priv->reg_lock); bcmgenet_umac_writel(priv, 0, UMAC_MDF_CTRL); return; } else { reg &= ~CMD_PROMISC; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock(&priv->reg_lock); } /* update MDF filter */ @@ -4005,6 +4014,7 @@ static int bcmgenet_probe(struct platform_device *pdev) goto err; } + spin_lock_init(&priv->reg_lock); spin_lock_init(&priv->lock); /* Set default pause parameters */ diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h index 7523b60b3c1c..43b923c48b14 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* - * Copyright (c) 2014-2020 Broadcom + * Copyright (c) 2014-2024 Broadcom */ #ifndef __BCMGENET_H__ @@ -573,6 +573,8 @@ struct bcmgenet_rxnfc_rule { /* device context */ struct bcmgenet_priv { void __iomem *base; + /* reg_lock: lock to serialize access to shared registers */ + spinlock_t reg_lock; enum bcmgenet_version version; struct net_device *dev; diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c index 7a41cad5788f..1248792d7fd4 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c @@ -2,7 +2,7 @@ /* * Broadcom GENET (Gigabit Ethernet) Wake-on-LAN support * - * Copyright (c) 2014-2020 Broadcom + * Copyright (c) 2014-2024 Broadcom */ #define pr_fmt(fmt) "bcmgenet_wol: " fmt @@ -151,6 +151,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, } /* Can't suspend with WoL if MAC is still in reset */ + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); if (reg & CMD_SW_RESET) reg &= ~CMD_SW_RESET; @@ -158,6 +159,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, /* disable RX */ reg &= ~CMD_RX_EN; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); mdelay(10); if (priv->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE)) { @@ -203,6 +205,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, } /* Enable CRC forward */ + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); priv->crc_fwd_en = 1; reg |= CMD_CRC_FWD; @@ -210,6 +213,7 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv, /* Receiver must be enabled for WOL MP detection */ reg |= CMD_RX_EN; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); reg = UMAC_IRQ_MPD_R; if (hfb_enable) @@ -256,7 +260,9 @@ void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv, } /* Disable CRC Forward */ + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); reg &= ~CMD_CRC_FWD; bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); } diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 86a4aa72b3d4..c4a3698cef66 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -76,6 +76,7 @@ static void bcmgenet_mac_config(struct net_device *dev) reg |= RGMII_LINK; bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL); + spin_lock_bh(&priv->reg_lock); reg = bcmgenet_umac_readl(priv, UMAC_CMD); reg &= ~((CMD_SPEED_MASK << CMD_SPEED_SHIFT) | CMD_HD_EN | @@ -88,6 +89,7 @@ static void bcmgenet_mac_config(struct net_device *dev) reg |= CMD_TX_EN | CMD_RX_EN; } bcmgenet_umac_writel(priv, reg, UMAC_CMD); + spin_unlock_bh(&priv->reg_lock); active = phy_init_eee(phydev, 0) >= 0; bcmgenet_eee_enable_set(dev, -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] net: bcmgenet: synchronize UMAC_CMD access 2024-04-25 22:10 ` [PATCH 3/3] net: bcmgenet: synchronize UMAC_CMD access Doug Berger @ 2024-04-27 14:16 ` Simon Horman 0 siblings, 0 replies; 8+ messages in thread From: Simon Horman @ 2024-04-27 14:16 UTC (permalink / raw) To: Doug Berger; +Cc: netdev, stable On Thu, Apr 25, 2024 at 03:10:07PM -0700, Doug Berger wrote: > The UMAC_CMD register is written from different execution > contexts and has insufficient synchronization protections to > prevent possible corruption. Of particular concern are the > acceses from the phy_device delayed work context used by the > adjust_link call and the BH context that may be used by the > ndo_set_rx_mode call. > > A spinlock is added to the driver to protect contended register > accesses (i.e. reg_lock) and it is used to synchronize accesses > to UMAC_CMD. > > Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file") > Cc: stable@vger.kernel.org > Signed-off-by: Doug Berger <opendmb@gmail.com> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 0/3] net: bcmgenet: protect contended accesses @ 2024-04-25 22:06 Doug Berger 2024-04-25 22:06 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger 0 siblings, 1 reply; 8+ messages in thread From: Doug Berger @ 2024-04-25 22:06 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Florian Fainelli, bcm-kernel-feedback-list, netdev, linux-kernel, Doug Berger Some registers may be modified by parallel execution contexts and require protections to prevent corruption. A review of the driver revealed the need for these additional protections. Doug Berger (3): net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() net: bcmgenet: synchronize UMAC_CMD access drivers/net/ethernet/broadcom/genet/bcmgenet.c | 16 ++++++++++++++-- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 4 +++- .../net/ethernet/broadcom/genet/bcmgenet_wol.c | 8 +++++++- drivers/net/ethernet/broadcom/genet/bcmmii.c | 6 +++++- 4 files changed, 29 insertions(+), 5 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access 2024-04-25 22:06 [PATCH 0/3] net: bcmgenet: protect contended accesses Doug Berger @ 2024-04-25 22:06 ` Doug Berger 0 siblings, 0 replies; 8+ messages in thread From: Doug Berger @ 2024-04-25 22:06 UTC (permalink / raw) To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Florian Fainelli, bcm-kernel-feedback-list, netdev, linux-kernel, Doug Berger, stable The EXT_RGMII_OOB_CTRL register can be written from different contexts. It is predominantly written from the adjust_link handler which is synchronized by the phydev->lock, but can also be written from a different context when configuring the mii in bcmgenet_mii_config(). The chances of contention are quite low, but it is conceivable that adjust_link could occur during resume when WoL is enabled so use the phydev->lock synchronizer in bcmgenet_mii_config() to be sure. Fixes: afe3f907d20f ("net: bcmgenet: power on MII block for all MII modes") Cc: stable@vger.kernel.org Signed-off-by: Doug Berger <opendmb@gmail.com> --- drivers/net/ethernet/broadcom/genet/bcmmii.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 9ada89355747..86a4aa72b3d4 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -2,7 +2,7 @@ /* * Broadcom GENET MDIO routines * - * Copyright (c) 2014-2017 Broadcom + * Copyright (c) 2014-2024 Broadcom */ #include <linux/acpi.h> @@ -275,6 +275,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) * block for the interface to work, unconditionally clear the * Out-of-band disable since we do not need it. */ + mutex_lock(&phydev->lock); reg = bcmgenet_ext_readl(priv, EXT_RGMII_OOB_CTRL); reg &= ~OOB_DISABLE; if (priv->ext_phy) { @@ -286,6 +287,7 @@ int bcmgenet_mii_config(struct net_device *dev, bool init) reg |= RGMII_MODE_EN; } bcmgenet_ext_writel(priv, reg, EXT_RGMII_OOB_CTRL); + mutex_unlock(&phydev->lock); if (init) dev_info(kdev, "configuring instance for %s\n", phy_name); -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-27 14:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-25 22:10 [PATCH 0/3] net: bcmgenet: protect contended accesses Doug Berger 2024-04-25 22:10 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger 2024-04-27 14:16 ` Simon Horman 2024-04-25 22:10 ` [PATCH 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() Doug Berger 2024-04-27 14:16 ` Simon Horman 2024-04-25 22:10 ` [PATCH 3/3] net: bcmgenet: synchronize UMAC_CMD access Doug Berger 2024-04-27 14:16 ` Simon Horman -- strict thread matches above, loose matches on Subject: below -- 2024-04-25 22:06 [PATCH 0/3] net: bcmgenet: protect contended accesses Doug Berger 2024-04-25 22:06 ` [PATCH 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.