* [PATCH net v1 1/4] octeon_ep: fix race conditions in ndo_get_stats64
2024-12-03 7:21 [PATCH net v1 0/4] Fix race conditions in ndo_get_stats64 Shinas Rasheed
@ 2024-12-03 7:21 ` Shinas Rasheed
2024-12-04 2:23 ` Jakub Kicinski
2024-12-03 7:21 ` [PATCH net v1 2/4] octeon_ep: remove firmware stats fetch " Shinas Rasheed
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Shinas Rasheed @ 2024-12-03 7:21 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: hgani, sedara, vimleshk, thaller, wizhao, kheib, egallen,
konguyen, horms, einstein.xue, Shinas Rasheed,
Veerasenareddy Burru, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Satananda Burla, Abhijit Ayarekar
ndo_get_stats64() can race with ndo_stop(), which frees input and
output queue resources. Implement device state variable to protect
against such resource usage.
Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
.../ethernet/marvell/octeon_ep/octep_main.c | 27 +++++++++++++++++++
.../ethernet/marvell/octeon_ep/octep_main.h | 8 ++++++
2 files changed, 35 insertions(+)
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 549436efc204..872b4848027c 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -733,6 +733,7 @@ static int octep_open(struct net_device *netdev)
if (ret > 0)
octep_link_up(netdev);
+ set_bit(OCTEP_DEV_STATE_OPEN, &oct->state);
return 0;
set_queues_err:
@@ -745,6 +746,11 @@ static int octep_open(struct net_device *netdev)
return -1;
}
+static bool octep_drv_busy(struct octep_device *oct)
+{
+ return test_bit(OCTEP_DEV_STATE_READ_STATS, &oct->state);
+}
+
/**
* octep_stop() - stop the octeon network device.
*
@@ -759,6 +765,14 @@ static int octep_stop(struct net_device *netdev)
netdev_info(netdev, "Stopping the device ...\n");
+ clear_bit(OCTEP_DEV_STATE_OPEN, &oct->state);
+ /* Make sure device state open is cleared so that no more
+ * stats fetch can happen intermittently
+ */
+ smp_mb__after_atomic();
+ while (octep_drv_busy(oct))
+ msleep(20);
+
octep_ctrl_net_set_link_status(oct, OCTEP_CTRL_NET_INVALID_VFID, false,
false);
octep_ctrl_net_set_rx_state(oct, OCTEP_CTRL_NET_INVALID_VFID, false,
@@ -1001,6 +1015,16 @@ static void octep_get_stats64(struct net_device *netdev,
&oct->iface_rx_stats,
&oct->iface_tx_stats);
+ set_bit(OCTEP_DEV_STATE_READ_STATS, &oct->state);
+ /* Make sure read stats state is set, so that ndo_stop
+ * doesn't clear resources as they are read
+ */
+ smp_mb__after_atomic();
+ if (!test_bit(OCTEP_DEV_STATE_OPEN, &oct->state)) {
+ clear_bit(OCTEP_DEV_STATE_READ_STATS, &oct->state);
+ return;
+ }
+
tx_packets = 0;
tx_bytes = 0;
rx_packets = 0;
@@ -1022,6 +1046,7 @@ static void octep_get_stats64(struct net_device *netdev,
stats->rx_errors = oct->iface_rx_stats.err_pkts;
stats->collisions = oct->iface_tx_stats.xscol;
stats->tx_fifo_errors = oct->iface_tx_stats.undflw;
+ clear_bit(OCTEP_DEV_STATE_READ_STATS, &oct->state);
}
/**
@@ -1526,6 +1551,8 @@ static int octep_sriov_disable(struct octep_device *oct)
pci_disable_sriov(pdev);
CFG_GET_ACTIVE_VFS(oct->conf) = 0;
+ clear_bit(OCTEP_DEV_STATE_OPEN, &oct->state);
+
return 0;
}
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
index fee59e0e0138..78293366e7de 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h
@@ -223,6 +223,12 @@ struct octep_pfvf_info {
u32 mbox_version;
};
+/* Device state */
+enum octep_dev_state {
+ OCTEP_DEV_STATE_OPEN,
+ OCTEP_DEV_STATE_READ_STATS,
+};
+
/* The Octeon device specific private data structure.
* Each Octeon device has this structure to represent all its components.
*/
@@ -318,6 +324,8 @@ struct octep_device {
atomic_t hb_miss_cnt;
/* Task to reset device on heartbeat miss */
struct delayed_work hb_task;
+ /* Device state */
+ unsigned long state;
};
static inline u16 OCTEP_MAJOR_REV(struct octep_device *oct)
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net v1 1/4] octeon_ep: fix race conditions in ndo_get_stats64
2024-12-03 7:21 ` [PATCH net v1 1/4] octeon_ep: fix " Shinas Rasheed
@ 2024-12-04 2:23 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-12-04 2:23 UTC (permalink / raw)
To: Shinas Rasheed
Cc: netdev, linux-kernel, hgani, sedara, vimleshk, thaller, wizhao,
kheib, egallen, konguyen, horms, einstein.xue,
Veerasenareddy Burru, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Satananda Burla, Abhijit Ayarekar
On Mon, 2 Dec 2024 23:21:27 -0800 Shinas Rasheed wrote:
> + clear_bit(OCTEP_DEV_STATE_OPEN, &oct->state);
> + /* Make sure device state open is cleared so that no more
> + * stats fetch can happen intermittently
> + */
> + smp_mb__after_atomic();
> + while (octep_drv_busy(oct))
> + msleep(20);
This is a poor re-implementation of a lock.
We have more lock types in Linux than I care to count now.
Please just use the right one. Hint, it's probably a spin lock or
RCU+synchronize_net() on close.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v1 2/4] octeon_ep: remove firmware stats fetch in ndo_get_stats64
2024-12-03 7:21 [PATCH net v1 0/4] Fix race conditions in ndo_get_stats64 Shinas Rasheed
2024-12-03 7:21 ` [PATCH net v1 1/4] octeon_ep: fix " Shinas Rasheed
@ 2024-12-03 7:21 ` Shinas Rasheed
2024-12-03 7:21 ` [PATCH net v1 3/4] octeon_ep_vf: fix race conditions " Shinas Rasheed
2024-12-03 7:21 ` [PATCH net v1 4/4] octeon_ep_vf: remove firmware stats fetch " Shinas Rasheed
3 siblings, 0 replies; 6+ messages in thread
From: Shinas Rasheed @ 2024-12-03 7:21 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: hgani, sedara, vimleshk, thaller, wizhao, kheib, egallen,
konguyen, horms, einstein.xue, Shinas Rasheed,
Veerasenareddy Burru, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Abhijit Ayarekar, Satananda Burla
The per queue stats are available already and are retrieved
from register reads during ndo_get_stats64. The firmware stats
fetch call that happens in ndo_get_stats64() is currently not
required
Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 872b4848027c..8c0a955a02dc 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1009,12 +1009,6 @@ static void octep_get_stats64(struct net_device *netdev,
struct octep_device *oct = netdev_priv(netdev);
int q;
- if (netif_running(netdev))
- octep_ctrl_net_get_if_stats(oct,
- OCTEP_CTRL_NET_INVALID_VFID,
- &oct->iface_rx_stats,
- &oct->iface_tx_stats);
-
set_bit(OCTEP_DEV_STATE_READ_STATS, &oct->state);
/* Make sure read stats state is set, so that ndo_stop
* doesn't clear resources as they are read
@@ -1042,11 +1036,6 @@ static void octep_get_stats64(struct net_device *netdev,
stats->tx_bytes = tx_bytes;
stats->rx_packets = rx_packets;
stats->rx_bytes = rx_bytes;
- stats->multicast = oct->iface_rx_stats.mcast_pkts;
- stats->rx_errors = oct->iface_rx_stats.err_pkts;
- stats->collisions = oct->iface_tx_stats.xscol;
- stats->tx_fifo_errors = oct->iface_tx_stats.undflw;
- clear_bit(OCTEP_DEV_STATE_READ_STATS, &oct->state);
}
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net v1 3/4] octeon_ep_vf: fix race conditions in ndo_get_stats64
2024-12-03 7:21 [PATCH net v1 0/4] Fix race conditions in ndo_get_stats64 Shinas Rasheed
2024-12-03 7:21 ` [PATCH net v1 1/4] octeon_ep: fix " Shinas Rasheed
2024-12-03 7:21 ` [PATCH net v1 2/4] octeon_ep: remove firmware stats fetch " Shinas Rasheed
@ 2024-12-03 7:21 ` Shinas Rasheed
2024-12-03 7:21 ` [PATCH net v1 4/4] octeon_ep_vf: remove firmware stats fetch " Shinas Rasheed
3 siblings, 0 replies; 6+ messages in thread
From: Shinas Rasheed @ 2024-12-03 7:21 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: hgani, sedara, vimleshk, thaller, wizhao, kheib, egallen,
konguyen, horms, einstein.xue, Shinas Rasheed,
Veerasenareddy Burru, Satananda Burla, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
ndo_get_stats64() can race with ndo_stop(), which frees input and
output queue resources. Implement device state variable to protect
against such resource usage.
Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
.../marvell/octeon_ep_vf/octep_vf_main.c | 29 +++++++++++++++++++
.../marvell/octeon_ep_vf/octep_vf_main.h | 9 ++++++
2 files changed, 38 insertions(+)
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
index 7e6771c9cdbb..12b95fb21e64 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
@@ -497,6 +497,8 @@ static int octep_vf_open(struct net_device *netdev)
if (ret)
octep_vf_link_up(netdev);
+ set_bit(OCTEP_VF_DEV_STATE_OPEN, &oct->state);
+
return 0;
set_queues_err:
@@ -511,6 +513,11 @@ static int octep_vf_open(struct net_device *netdev)
return -1;
}
+static bool octep_vf_drv_busy(struct octep_vf_device *oct)
+{
+ return test_bit(OCTEP_VF_DEV_STATE_READ_STATS, &oct->state);
+}
+
/**
* octep_vf_stop() - stop the octeon network device.
*
@@ -525,6 +532,14 @@ static int octep_vf_stop(struct net_device *netdev)
netdev_info(netdev, "Stopping the device ...\n");
+ clear_bit(OCTEP_VF_DEV_STATE_OPEN, &oct->state);
+ /* Make sure device state open is cleared so that no more
+ * stats fetch can happen intermittently
+ */
+ smp_mb__after_atomic();
+ while (octep_vf_drv_busy(oct))
+ msleep(20);
+
/* Stop Tx from stack */
netif_carrier_off(netdev);
netif_tx_disable(netdev);
@@ -782,6 +797,16 @@ static void octep_vf_get_stats64(struct net_device *netdev,
u64 tx_packets, tx_bytes, rx_packets, rx_bytes;
int q;
+ set_bit(OCTEP_VF_DEV_STATE_READ_STATS, &oct->state);
+ /* Make sure read stats state is set, so that ndo_stop
+ * doesn't clear resources as they are read
+ */
+ smp_mb__after_atomic();
+ if (!test_bit(OCTEP_VF_DEV_STATE_OPEN, &oct->state)) {
+ clear_bit(OCTEP_VF_DEV_STATE_READ_STATS, &oct->state);
+ return;
+ }
+
tx_packets = 0;
tx_bytes = 0;
rx_packets = 0;
@@ -807,6 +832,7 @@ static void octep_vf_get_stats64(struct net_device *netdev,
stats->rx_missed_errors = oct->iface_rx_stats.dropped_pkts_fifo_full;
stats->tx_dropped = oct->iface_tx_stats.dropped;
}
+ clear_bit(OCTEP_VF_DEV_STATE_READ_STATS, &oct->state);
}
/**
@@ -1140,6 +1166,9 @@ static int octep_vf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
dev_err(&pdev->dev, "Failed to register netdev\n");
goto delete_mbox;
}
+
+ clear_bit(OCTEP_VF_DEV_STATE_OPEN, &octep_vf_dev->state);
+
dev_info(&pdev->dev, "Device probe successful\n");
return 0;
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h
index 5769f62545cd..692b3bf94722 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.h
@@ -213,6 +213,12 @@ struct octep_vf_fw_info {
u16 tx_ol_flags;
};
+/* Device state */
+enum octep_vf_dev_state {
+ OCTEP_VF_DEV_STATE_OPEN,
+ OCTEP_VF_DEV_STATE_READ_STATS,
+};
+
/* The Octeon device specific private data structure.
* Each Octeon device has this structure to represent all its components.
*/
@@ -282,6 +288,9 @@ struct octep_vf_device {
/* offset for iface stats */
u32 ctrl_mbox_ifstats_offset;
+ /* Device state */
+ unsigned long state;
+
/* Negotiated Mbox version */
u32 mbox_neg_ver;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net v1 4/4] octeon_ep_vf: remove firmware stats fetch in ndo_get_stats64
2024-12-03 7:21 [PATCH net v1 0/4] Fix race conditions in ndo_get_stats64 Shinas Rasheed
` (2 preceding siblings ...)
2024-12-03 7:21 ` [PATCH net v1 3/4] octeon_ep_vf: fix race conditions " Shinas Rasheed
@ 2024-12-03 7:21 ` Shinas Rasheed
3 siblings, 0 replies; 6+ messages in thread
From: Shinas Rasheed @ 2024-12-03 7:21 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: hgani, sedara, vimleshk, thaller, wizhao, kheib, egallen,
konguyen, horms, einstein.xue, Shinas Rasheed,
Veerasenareddy Burru, Satananda Burla, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
The per queue stats are available already and are retrieved
from register reads during ndo_get_stats64. The firmware stats
fetch call that happens in ndo_get_stats64() is currently not
required
Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
index 12b95fb21e64..38e93d79ae59 100644
--- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c
@@ -824,14 +824,6 @@ static void octep_vf_get_stats64(struct net_device *netdev,
stats->tx_bytes = tx_bytes;
stats->rx_packets = rx_packets;
stats->rx_bytes = rx_bytes;
- if (!octep_vf_get_if_stats(oct)) {
- stats->multicast = oct->iface_rx_stats.mcast_pkts;
- stats->rx_errors = oct->iface_rx_stats.err_pkts;
- stats->rx_dropped = oct->iface_rx_stats.dropped_pkts_fifo_full +
- oct->iface_rx_stats.err_pkts;
- stats->rx_missed_errors = oct->iface_rx_stats.dropped_pkts_fifo_full;
- stats->tx_dropped = oct->iface_tx_stats.dropped;
- }
clear_bit(OCTEP_VF_DEV_STATE_READ_STATS, &oct->state);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread