* [PATCH net 1/3] bnxt_en: Fix bnxt_reset() in the slow path task.
2017-01-25 7:55 [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task() Michael Chan
@ 2017-01-25 7:55 ` Michael Chan
2017-01-25 7:55 ` [PATCH net 2/3] bnxt_en: Fix RTNL lock usage on bnxt_update_link() Michael Chan
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Michael Chan @ 2017-01-25 7:55 UTC (permalink / raw)
To: davem; +Cc: netdev
In bnxt_sp_task(), we set a bit BNXT_STATE_IN_SP_TASK so that bnxt_close()
will synchronize and wait for bnxt_sp_task() to finish. Some functions
in bnxt_sp_task() require us to clear BNXT_STATE_IN_SP_TASK and then
acquire rtnl_lock() to prevent race conditions.
There are some bugs related to this logic. This patch refactors the code
to have common bnxt_rtnl_lock_sp() and bnxt_rtnl_unlock_sp() to handle
the RTNL and the clearing/setting of the bit. Multiple functions will
need the same logic. We also need to move bnxt_reset() to the end of
bnxt_sp_task(). Functions that clear BNXT_STATE_IN_SP_TASK must be the
last functions to be called in bnxt_sp_task(). The common scheme will
handle the condition properly.
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 38 ++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 53e686f..30d7d64 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6200,23 +6200,32 @@ static void bnxt_timer(unsigned long data)
mod_timer(&bp->timer, jiffies + bp->current_interval);
}
-/* Only called from bnxt_sp_task() */
-static void bnxt_reset(struct bnxt *bp, bool silent)
+static void bnxt_rtnl_lock_sp(struct bnxt *bp)
{
- /* bnxt_reset_task() calls bnxt_close_nic() which waits
- * for BNXT_STATE_IN_SP_TASK to clear.
- * If there is a parallel dev_close(), bnxt_close() may be holding
+ /* We are called from bnxt_sp_task which has BNXT_STATE_IN_SP_TASK
+ * set. If the device is being closed, bnxt_close() may be holding
* rtnl() and waiting for BNXT_STATE_IN_SP_TASK to clear. So we
* must clear BNXT_STATE_IN_SP_TASK before holding rtnl().
*/
clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
rtnl_lock();
- if (test_bit(BNXT_STATE_OPEN, &bp->state))
- bnxt_reset_task(bp, silent);
+}
+
+static void bnxt_rtnl_unlock_sp(struct bnxt *bp)
+{
set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
rtnl_unlock();
}
+/* Only called from bnxt_sp_task() */
+static void bnxt_reset(struct bnxt *bp, bool silent)
+{
+ bnxt_rtnl_lock_sp(bp);
+ if (test_bit(BNXT_STATE_OPEN, &bp->state))
+ bnxt_reset_task(bp, silent);
+ bnxt_rtnl_unlock_sp(bp);
+}
+
static void bnxt_cfg_ntp_filters(struct bnxt *);
static void bnxt_sp_task(struct work_struct *work)
@@ -6266,18 +6275,21 @@ static void bnxt_sp_task(struct work_struct *work)
bnxt_hwrm_tunnel_dst_port_free(
bp, TUNNEL_DST_PORT_FREE_REQ_TUNNEL_TYPE_GENEVE);
}
- if (test_and_clear_bit(BNXT_RESET_TASK_SP_EVENT, &bp->sp_event))
- bnxt_reset(bp, false);
-
- if (test_and_clear_bit(BNXT_RESET_TASK_SILENT_SP_EVENT, &bp->sp_event))
- bnxt_reset(bp, true);
-
if (test_and_clear_bit(BNXT_HWRM_PORT_MODULE_SP_EVENT, &bp->sp_event))
bnxt_get_port_module_status(bp);
if (test_and_clear_bit(BNXT_PERIODIC_STATS_SP_EVENT, &bp->sp_event))
bnxt_hwrm_port_qstats(bp);
+ /* These functions below will clear BNXT_STATE_IN_SP_TASK. They
+ * must be the last functions to be called before exiting.
+ */
+ if (test_and_clear_bit(BNXT_RESET_TASK_SP_EVENT, &bp->sp_event))
+ bnxt_reset(bp, false);
+
+ if (test_and_clear_bit(BNXT_RESET_TASK_SILENT_SP_EVENT, &bp->sp_event))
+ bnxt_reset(bp, true);
+
smp_mb__before_atomic();
clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net 2/3] bnxt_en: Fix RTNL lock usage on bnxt_update_link().
2017-01-25 7:55 [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task() Michael Chan
2017-01-25 7:55 ` [PATCH net 1/3] bnxt_en: Fix bnxt_reset() in the slow path task Michael Chan
@ 2017-01-25 7:55 ` Michael Chan
2017-01-25 7:55 ` [PATCH net 3/3] bnxt_en: Fix RTNL lock usage on bnxt_get_port_module_status() Michael Chan
2017-01-25 18:27 ` [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task() David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Michael Chan @ 2017-01-25 7:55 UTC (permalink / raw)
To: davem; +Cc: netdev
bnxt_update_link() is called from multiple code paths. Most callers,
such as open, ethtool, already hold RTNL. Only the caller bnxt_sp_task()
does not. So it is a bug to take RTNL inside bnxt_update_link().
Fix it by removing the RTNL inside bnxt_update_link(). The function
now expects the caller to always hold RTNL.
In bnxt_sp_task(), call bnxt_rtnl_lock_sp() before calling
bnxt_update_link(). We also need to move the call to the end of
bnxt_sp_task() since it will be clearing the BNXT_STATE_IN_SP_TASK bit.
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 37 +++++++++++++++----------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 30d7d64..69b0fce 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5314,17 +5314,12 @@ static int bnxt_update_link(struct bnxt *bp, bool chng_link_state)
if ((link_info->support_auto_speeds | diff) !=
link_info->support_auto_speeds) {
/* An advertised speed is no longer supported, so we need to
- * update the advertisement settings. See bnxt_reset() for
- * comments about the rtnl_lock() sequence below.
+ * update the advertisement settings. Caller holds RTNL
+ * so we can modify link settings.
*/
- clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
- rtnl_lock();
link_info->advertising = link_info->support_auto_speeds;
- if (test_bit(BNXT_STATE_OPEN, &bp->state) &&
- (link_info->autoneg & BNXT_AUTONEG_SPEED))
+ if (link_info->autoneg & BNXT_AUTONEG_SPEED)
bnxt_hwrm_set_link_setting(bp, true, false);
- set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
- rtnl_unlock();
}
return 0;
}
@@ -6231,7 +6226,6 @@ static void bnxt_reset(struct bnxt *bp, bool silent)
static void bnxt_sp_task(struct work_struct *work)
{
struct bnxt *bp = container_of(work, struct bnxt, sp_task);
- int rc;
set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
smp_mb__after_atomic();
@@ -6245,16 +6239,6 @@ static void bnxt_sp_task(struct work_struct *work)
if (test_and_clear_bit(BNXT_RX_NTP_FLTR_SP_EVENT, &bp->sp_event))
bnxt_cfg_ntp_filters(bp);
- if (test_and_clear_bit(BNXT_LINK_CHNG_SP_EVENT, &bp->sp_event)) {
- if (test_and_clear_bit(BNXT_LINK_SPEED_CHNG_SP_EVENT,
- &bp->sp_event))
- bnxt_hwrm_phy_qcaps(bp);
-
- rc = bnxt_update_link(bp, true);
- if (rc)
- netdev_err(bp->dev, "SP task can't update link (rc: %x)\n",
- rc);
- }
if (test_and_clear_bit(BNXT_HWRM_EXEC_FWD_REQ_SP_EVENT, &bp->sp_event))
bnxt_hwrm_exec_fwd_req(bp);
if (test_and_clear_bit(BNXT_VXLAN_ADD_PORT_SP_EVENT, &bp->sp_event)) {
@@ -6284,6 +6268,21 @@ static void bnxt_sp_task(struct work_struct *work)
/* These functions below will clear BNXT_STATE_IN_SP_TASK. They
* must be the last functions to be called before exiting.
*/
+ if (test_and_clear_bit(BNXT_LINK_CHNG_SP_EVENT, &bp->sp_event)) {
+ int rc = 0;
+
+ if (test_and_clear_bit(BNXT_LINK_SPEED_CHNG_SP_EVENT,
+ &bp->sp_event))
+ bnxt_hwrm_phy_qcaps(bp);
+
+ bnxt_rtnl_lock_sp(bp);
+ if (test_bit(BNXT_STATE_OPEN, &bp->state))
+ rc = bnxt_update_link(bp, true);
+ bnxt_rtnl_unlock_sp(bp);
+ if (rc)
+ netdev_err(bp->dev, "SP task can't update link (rc: %x)\n",
+ rc);
+ }
if (test_and_clear_bit(BNXT_RESET_TASK_SP_EVENT, &bp->sp_event))
bnxt_reset(bp, false);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net 3/3] bnxt_en: Fix RTNL lock usage on bnxt_get_port_module_status().
2017-01-25 7:55 [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task() Michael Chan
2017-01-25 7:55 ` [PATCH net 1/3] bnxt_en: Fix bnxt_reset() in the slow path task Michael Chan
2017-01-25 7:55 ` [PATCH net 2/3] bnxt_en: Fix RTNL lock usage on bnxt_update_link() Michael Chan
@ 2017-01-25 7:55 ` Michael Chan
2017-01-25 18:27 ` [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task() David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Michael Chan @ 2017-01-25 7:55 UTC (permalink / raw)
To: davem; +Cc: netdev
bnxt_get_port_module_status() calls bnxt_update_link() which expects
RTNL to be held. In bnxt_sp_task() that does not hold RTNL, we need to
call it with a prior call to bnxt_rtnl_lock_sp() and the call needs to
be moved to the end of bnxt_sp_task().
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 69b0fce..4fcc6a8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6259,9 +6259,6 @@ static void bnxt_sp_task(struct work_struct *work)
bnxt_hwrm_tunnel_dst_port_free(
bp, TUNNEL_DST_PORT_FREE_REQ_TUNNEL_TYPE_GENEVE);
}
- if (test_and_clear_bit(BNXT_HWRM_PORT_MODULE_SP_EVENT, &bp->sp_event))
- bnxt_get_port_module_status(bp);
-
if (test_and_clear_bit(BNXT_PERIODIC_STATS_SP_EVENT, &bp->sp_event))
bnxt_hwrm_port_qstats(bp);
@@ -6283,6 +6280,12 @@ static void bnxt_sp_task(struct work_struct *work)
netdev_err(bp->dev, "SP task can't update link (rc: %x)\n",
rc);
}
+ if (test_and_clear_bit(BNXT_HWRM_PORT_MODULE_SP_EVENT, &bp->sp_event)) {
+ bnxt_rtnl_lock_sp(bp);
+ if (test_bit(BNXT_STATE_OPEN, &bp->state))
+ bnxt_get_port_module_status(bp);
+ bnxt_rtnl_unlock_sp(bp);
+ }
if (test_and_clear_bit(BNXT_RESET_TASK_SP_EVENT, &bp->sp_event))
bnxt_reset(bp, false);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task().
2017-01-25 7:55 [PATCH net 0/3] bnxt_en: Fix RTNL lock usage in bnxt_sp_task() Michael Chan
` (2 preceding siblings ...)
2017-01-25 7:55 ` [PATCH net 3/3] bnxt_en: Fix RTNL lock usage on bnxt_get_port_module_status() Michael Chan
@ 2017-01-25 18:27 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-01-25 18:27 UTC (permalink / raw)
To: michael.chan; +Cc: netdev
From: Michael Chan <michael.chan@broadcom.com>
Date: Wed, 25 Jan 2017 02:55:06 -0500
> There are 2 function calls from bnxt_sp_task() that have buggy RTNL
> usage. These 2 functions take RTNL lock under some conditions, but
> some callers (such as open, ethtool) have already taken RTNL. These
> 3 patches fix the issue by making it clear that callers must take
> RTNL. If the caller is bnxt_sp_task() which does not automatically
> take RTNL, we add a common scheme for bnxt_sp_task() to call these
> functions properly under RTNL.
Series applied, thanks Michael.
You can sprinkle some ASSERT_RTNL(); in the functions that require
RTNL to be held if you like.
^ permalink raw reply [flat|nested] 5+ messages in thread