[PATCH net v4 0/4] Fix race conditions in ndo_get_stats64

[PATCH net v4 0/4] Fix race conditions in ndo_get_stats64

[Shinas Rasheed]

Fix race conditions in ndo_get_stats64 by checking if netdev is running
before per queue resources are accessed, and remove stats fetch from
firmware which is currently unnecessary

Changes:
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-1-srasheed@marvell.com/
  - Added warn log that happened due to rcu_read_lock in commit message

V2: https://lore.kernel.org/all/20241216075842.2394606-1-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-1-srasheed@marvell.com/

Shinas Rasheed (4):
  octeon_ep: fix race conditions in ndo_get_stats64
  octeon_ep: remove firmware stats fetch in ndo_get_stats64
  octeon_ep_vf: fix race conditions in ndo_get_stats64
  octeon_ep_vf: remove firmware stats fetch in ndo_get_stats64

 .../net/ethernet/marvell/octeon_ep/octep_main.c    | 14 ++++----------
 .../ethernet/marvell/octeon_ep_vf/octep_vf_main.c  | 12 ++++--------
 2 files changed, 8 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH net v4 1/4] octeon_ep: fix race conditions in ndo_get_stats64

[Shinas Rasheed]

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(-)

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];
-- 
2.25.1

Re: [PATCH net v4 1/4] octeon_ep: fix race conditions in ndo_get_stats64

[Jakub Kicinski]

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..
-- 
pw-bot: cr

RE: [EXTERNAL] Re: [PATCH net v4 1/4] octeon_ep: fix race conditions in ndo_get_stats64

[Shinas Rasheed]

Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Saturday, January 4, 2025 10:31 PM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Haseeb Gani
> <hgani@marvell.com>; Sathesh B Edara <sedara@marvell.com>; Vimlesh
> Kumar <vimleshk@marvell.com>; thaller@redhat.com; wizhao@redhat.com;
> kheib@redhat.com; konguyen@redhat.com; horms@kernel.org;
> einstein.xue@synaxg.com; Veerasenareddy Burru <vburru@marvell.com>;
> Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo
> Abeni <pabeni@redhat.com>; Abhijit Ayarekar <aayarekar@marvell.com>;
> Satananda Burla <sburla@marvell.com>
> Subject: [EXTERNAL] Re: [PATCH net v4 1/4] octeon_ep: fix race conditions in
> ndo_get_stats64
> 
> 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
> 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..

I ACK the 'save stats from freed queues and emit out stats when device is down'.

About the synchronization, the reason I changed to simple netif_running check was to avoid
locks (as per previous patch version comments). Please do correct me if I'm wrong, but isn't the case
you mentioned protected by the rtnl_lock held by the netdev stack when it calls the ndo_op ?

> --
> pw-bot: cr

Re: [EXTERNAL] Re: [PATCH net v4 1/4] octeon_ep: fix race conditions in ndo_get_stats64

[Jakub Kicinski]

On Mon, 6 Jan 2025 05:57:09 +0000 Shinas Rasheed wrote:
> > >  	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..  
> 
> I ACK the 'save stats from freed queues and emit out stats when device is down'.
> 
> About the synchronization, the reason I changed to simple netif_running check was to avoid
> locks (as per previous patch version comments). Please do correct me if I'm wrong, but isn't the case
> you mentioned protected by the rtnl_lock held by the netdev stack when it calls the ndo_op ?

I don't see rtnl_lock being taken in the procfs path.

FWIW I posted a test for the problem you're fixing in octeon, 
since it's relatively common among drivers:
 https://lore.kernel.org/20250105011525.1718380-1-kuba@kernel.org
see also:
 https://github.com/linux-netdev/nipa/wiki/Running-driver-tests

RE: [EXTERNAL] Re: [PATCH net v4 1/4] octeon_ep: fix race conditions in ndo_get_stats64

[Shinas Rasheed]

Hi Jakub,

Thanks for the reply, will revert

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, January 7, 2025 2:27 AM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Haseeb Gani
> <hgani@marvell.com>; Sathesh B Edara <sedara@marvell.com>; Vimlesh
> Kumar <vimleshk@marvell.com>; thaller@redhat.com; wizhao@redhat.com;
> kheib@redhat.com; konguyen@redhat.com; horms@kernel.org;
> einstein.xue@synaxg.com; Veerasenareddy Burru <vburru@marvell.com>;
> Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo
> Abeni <pabeni@redhat.com>; Abhijit Ayarekar <aayarekar@marvell.com>;
> Satananda Burla <sburla@marvell.com>
> Subject: Re: [EXTERNAL] Re: [PATCH net v4 1/4] octeon_ep: fix race conditions
> in ndo_get_stats64
> 
> On Mon, 6 Jan 2025 05: 57: 09 +0000 Shinas Rasheed wrote: > > > struct
> octep_device *oct = netdev_priv(netdev); > > > int q; > > > > > > - if
> (netif_running(netdev)) > > > - octep_ctrl_net_get_if_stats(oct,
> On Mon, 6 Jan 2025 05:57:09 +0000 Shinas Rasheed wrote:
> > > >  	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..
> >
> > I ACK the 'save stats from freed queues and emit out stats when device is
> down'.
> >
> > About the synchronization, the reason I changed to simple netif_running
> check was to avoid
> > locks (as per previous patch version comments). Please do correct me if I'm
> wrong, but isn't the case
> > you mentioned protected by the rtnl_lock held by the netdev stack when it
> calls the ndo_op ?
> 
> I don't see rtnl_lock being taken in the procfs path.
> 
> FWIW I posted a test for the problem you're fixing in octeon,
> since it's relatively common among drivers:
>  https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lore.kernel.org_20250105011525.1718380-2D1-2Dkuba-
> 40kernel.org&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-
> oxrlgQ1rjXgWtmLz1pnaDjD96sDq-
> cKUwK4&m=9gsH3cuOJoFpbgNiQc2gqY6_Cugh5GeBCKFU9mmblsBxpslPW2q
> VVBa1LG7w8qmb&s=-9Gao3oSw4wAp6L8V86hli4Bmqu3Po8jfOqNOtYwL-
> o&e=
> see also:
>  https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_linux-
> 2Dnetdev_nipa_wiki_Running-2Ddriver-
> 2Dtests&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-
> oxrlgQ1rjXgWtmLz1pnaDjD96sDq-
> cKUwK4&m=9gsH3cuOJoFpbgNiQc2gqY6_Cugh5GeBCKFU9mmblsBxpslPW2q
> VVBa1LG7w8qmb&s=q8uPNNae_-
> 4ps18BT6XOel9HsYApsxh4IN01HF2_ARw&e=

[PATCH net v4 2/4] octeon_ep: remove firmware stats fetch in ndo_get_stats64

[Shinas Rasheed]

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

The warn log is given below:

[  123.316837] ------------[ cut here ]------------
[  123.316840] Voluntary context switch within RCU read-side critical section!
[  123.316917] pc : rcu_note_context_switch+0x2e4/0x300
[  123.316919] lr : rcu_note_context_switch+0x2e4/0x300
[  123.316947] Call trace:
[  123.316949]  rcu_note_context_switch+0x2e4/0x300
[  123.316952]  __schedule+0x84/0x584
[  123.316955]  schedule+0x38/0x90
[  123.316956]  schedule_timeout+0xa0/0x1d4
[  123.316959]  octep_send_mbox_req+0x190/0x230 [octeon_ep]
[  123.316966]  octep_ctrl_net_get_if_stats+0x78/0x100 [octeon_ep]
[  123.316970]  octep_get_stats64+0xd4/0xf0 [octeon_ep]
[  123.316975]  dev_get_stats+0x4c/0x114
[  123.316977]  dev_seq_printf_stats+0x3c/0x11c
[  123.316980]  dev_seq_show+0x1c/0x40
[  123.316982]  seq_read_iter+0x3cc/0x4e0
[  123.316985]  seq_read+0xc8/0x110
[  123.316987]  proc_reg_read+0x9c/0xec
[  123.316990]  vfs_read+0xc8/0x2ec
[  123.316993]  ksys_read+0x70/0x100
[  123.316995]  __arm64_sys_read+0x20/0x30
[  123.316997]  invoke_syscall.constprop.0+0x7c/0xd0
[  123.317000]  do_el0_svc+0xb4/0xd0
[  123.317002]  el0_svc+0xe8/0x1f4
[  123.317005]  el0t_64_sync_handler+0x134/0x150
[  123.317006]  el0t_64_sync+0x17c/0x180
[  123.317008] ---[ end trace 63399811432ab69b ]---

Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V4:
  - No changes

V3: https://lore.kernel.org/all/20241218115111.2407958-3-srasheed@marvell.com/
  - Added warn log that happened due to rcu_read_lock in commit message

V2: https://lore.kernel.org/all/20241216075842.2394606-3-srasheed@marvell.com/
  - No changes

V1: https://lore.kernel.org/all/20241203072130.2316913-3-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index a452ee3b9a98..59e2f8d01954 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1016,10 +1016,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;
 }
 
 /**
-- 
2.25.1

[PATCH net v4 3/4] octeon_ep_vf: fix race conditions in ndo_get_stats64

[Shinas Rasheed]

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: c3fad23cdc06 ("octeon_ep_vf: 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-4-srasheed@marvell.com/
  - No changes

V2: https://lore.kernel.org/all/20241216075842.2394606-4-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-4-srasheed@marvell.com/

 drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c | 4 ++++
 1 file changed, 4 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..9b29cd698361 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
@@ -786,6 +786,10 @@ static void octep_vf_get_stats64(struct net_device *netdev,
 	tx_bytes = 0;
 	rx_packets = 0;
 	rx_bytes = 0;
+
+	if (!netif_running(netdev))
+		return;
+
 	for (q = 0; q < oct->num_oqs; q++) {
 		struct octep_vf_iq *iq = oct->iq[q];
 		struct octep_vf_oq *oq = oct->oq[q];
-- 
2.25.1

[PATCH net v4 4/4] octeon_ep_vf: remove firmware stats fetch in ndo_get_stats64

[Shinas Rasheed]

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

The warn log is given below:

[  123.316837] ------------[ cut here ]------------
[  123.316840] Voluntary context switch within RCU read-side critical section!
[  123.316917] pc : rcu_note_context_switch+0x2e4/0x300
[  123.316919] lr : rcu_note_context_switch+0x2e4/0x300
[  123.316947] Call trace:
[  123.316949]  rcu_note_context_switch+0x2e4/0x300
[  123.316952]  __schedule+0x84/0x584
[  123.316955]  schedule+0x38/0x90
[  123.316956]  schedule_timeout+0xa0/0x1d4
[  123.316959]  octep_send_mbox_req+0x190/0x230 [octeon_ep]
[  123.316966]  octep_ctrl_net_get_if_stats+0x78/0x100 [octeon_ep]
[  123.316970]  octep_get_stats64+0xd4/0xf0 [octeon_ep]
[  123.316975]  dev_get_stats+0x4c/0x114
[  123.316977]  dev_seq_printf_stats+0x3c/0x11c
[  123.316980]  dev_seq_show+0x1c/0x40
[  123.316982]  seq_read_iter+0x3cc/0x4e0
[  123.316985]  seq_read+0xc8/0x110
[  123.316987]  proc_reg_read+0x9c/0xec
[  123.316990]  vfs_read+0xc8/0x2ec
[  123.316993]  ksys_read+0x70/0x100
[  123.316995]  __arm64_sys_read+0x20/0x30
[  123.316997]  invoke_syscall.constprop.0+0x7c/0xd0
[  123.317000]  do_el0_svc+0xb4/0xd0
[  123.317002]  el0_svc+0xe8/0x1f4
[  123.317005]  el0t_64_sync_handler+0x134/0x150
[  123.317006]  el0t_64_sync+0x17c/0x180
[  123.317008] ---[ end trace 63399811432ab69b ]---

Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V4:
  - No changes

V3: https://lore.kernel.org/all/20241218115111.2407958-5-srasheed@marvell.com/
  - Added warn log that happened due to rcu_read_lock in commit message

V2: https://lore.kernel.org/all/20241216075842.2394606-5-srasheed@marvell.com/
  - No changes

V1: https://lore.kernel.org/all/20241203072130.2316913-5-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 9b29cd698361..33548f04497c 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
@@ -803,14 +803,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;
-	}
 }
 
 /**
-- 
2.25.1