| Message ID | 20250102112246.2494230-2-srasheed@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix race conditions in ndo_get_stats64 | expand |
On Thu, 2 Jan 2025 03:22:43 -0800 Shinas Rasheed wrote: > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c > index 549436efc204..a452ee3b9a98 100644 > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c > @@ -995,16 +995,14 @@ 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); > - > tx_packets = 0; > tx_bytes = 0; > rx_packets = 0; > rx_bytes = 0; > + > + if (!netif_running(netdev)) > + return; So we'll provide no stats when the device is down? That's not correct. The driver should save the stats from the freed queues (somewhere in the oct structure). Also please mention how this is synchronized against netif_running() changing its state, device may get closed while we're running..
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c index 549436efc204..a452ee3b9a98 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c @@ -995,16 +995,14 @@ 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); - tx_packets = 0; tx_bytes = 0; rx_packets = 0; rx_bytes = 0; + + if (!netif_running(netdev)) + return; + for (q = 0; q < oct->num_oqs; q++) { struct octep_iq *iq = oct->iq[q]; struct octep_oq *oq = oct->oq[q];
ndo_get_stats64() can race with ndo_stop(), which frees input and output queue resources. Check if netdev is running before accessing per queue resources. Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops") Signed-off-by: Shinas Rasheed <srasheed@marvell.com> --- V4: - Check if netdev is running, as decision for accessing resources rather than availing lock implementations, in ndo_get_stats64() V3: https://lore.kernel.org/all/20241218115111.2407958-2-srasheed@marvell.com/ - No changes V2: https://lore.kernel.org/all/20241216075842.2394606-2-srasheed@marvell.com/ - Changed sync mechanism to fix race conditions from using an atomic set_bit ops to a much simpler synchronize_net() V1: https://lore.kernel.org/all/20241203072130.2316913-2-srasheed@marvell.com/ drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)