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

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

[Shinas Rasheed]

Fix race conditions in ndo_get_stats64 by implementing a state variable
check, and remove unnecessary firmware stats fetch which is currently
unnecessary

Changes:
V2:
  - 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

 drivers/net/ethernet/marvell/octeon_ep/octep_main.c   | 11 +----------
 .../net/ethernet/marvell/octeon_ep_vf/octep_vf_main.c |  9 +--------
 2 files changed, 2 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH net v2 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. Call synchronize_net() to avoid such races.

Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 549436efc204..941bbaaa67b5 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -757,6 +757,7 @@ static int octep_stop(struct net_device *netdev)
 {
 	struct octep_device *oct = netdev_priv(netdev);
 
+	synchronize_net();
 	netdev_info(netdev, "Stopping the device ...\n");
 
 	octep_ctrl_net_set_link_status(oct, OCTEP_CTRL_NET_INVALID_VFID, false,
-- 
2.25.1

[PATCH net v2 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

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

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

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

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 941bbaaa67b5..6400d6008097 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -996,12 +996,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);
-
 	tx_packets = 0;
 	tx_bytes = 0;
 	rx_packets = 0;
@@ -1019,10 +1013,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 v2 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. Call synchornize_net() in ndo_stop()
to avoid such races.

Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - 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 | 1 +
 1 file changed, 1 insertion(+)

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..e6253f85b623 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
@@ -523,6 +523,7 @@ static int octep_vf_stop(struct net_device *netdev)
 {
 	struct octep_vf_device *oct = netdev_priv(netdev);
 
+	synchronize_rcu();
 	netdev_info(netdev, "Stopping the device ...\n");
 
 	/* Stop Tx from stack */
-- 
2.25.1

[PATCH net v2 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

Fixes: c3fad23cdc06 ("octeon_ep_vf: add support for ndo ops")
Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
---
V2:
  - 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 e6253f85b623..75dd3bd2b9ba 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
@@ -800,14 +800,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

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

[Larysa Zaremba]

On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> ndo_get_stats64() can race with ndo_stop(), which frees input and
> output queue resources. Call synchronize_net() to avoid such races.
> 
> Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> ---
> V2:
>   - 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 549436efc204..941bbaaa67b5 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -757,6 +757,7 @@ static int octep_stop(struct net_device *netdev)
>  {
>  	struct octep_device *oct = netdev_priv(netdev);
>  
> +	synchronize_net();

You should have elaborated on the fact that this synchronize_net() is for 
__LINK_STATE_START flag in the commit message, this is not obvious. Also, is 
octep_get_stats64() called from RCU-safe context?

>  	netdev_info(netdev, "Stopping the device ...\n");
>  
>  	octep_ctrl_net_set_link_status(oct, OCTEP_CTRL_NET_INVALID_VFID, false,
> -- 
> 2.25.1
> 
> 

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

[Larysa Zaremba]

On Sun, Dec 15, 2024 at 11:58:40PM -0800, Shinas Rasheed wrote:
> 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>
> ---
> V2:
>   - No changes
> 
> V1: https://lore.kernel.org/all/20241203072130.2316913-3-srasheed@marvell.com/
> 
>  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 941bbaaa67b5..6400d6008097 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -996,12 +996,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);
> -
>  	tx_packets = 0;
>  	tx_bytes = 0;
>  	rx_packets = 0;
> @@ -1019,10 +1013,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;

I do not see, how it is a fix to remove some fields from stats. If this is a 
cleanup, it should not go to the stable tree.

>  }
>  
>  /**
> -- 
> 2.25.1
> 
> 

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

[Larysa Zaremba]

On Mon, Dec 16, 2024 at 03:30:12PM +0100, Larysa Zaremba wrote:
> On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > output queue resources. Call synchronize_net() to avoid such races.
> > 
> > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > ---
> > V2:
> >   - 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 | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > index 549436efc204..941bbaaa67b5 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device *netdev)
> >  {
> >  	struct octep_device *oct = netdev_priv(netdev);
> >  
> > +	synchronize_net();
> 
> You should have elaborated on the fact that this synchronize_net() is for 
> __LINK_STATE_START flag in the commit message, this is not obvious. Also, is 
> octep_get_stats64() called from RCU-safe context?
>

Now I see that in case !netif_running(), you do not bail out of 
octep_get_stats64() fully (or at all after the second patch). So, could you 
explain, how are you utilizing RCU here?

> >  	netdev_info(netdev, "Stopping the device ...\n");
> >  
> >  	octep_ctrl_net_set_link_status(oct, OCTEP_CTRL_NET_INVALID_VFID, false,
> > -- 
> > 2.25.1
> > 
> > 
> 

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

[Shinas Rasheed]

Hi Larysa,

> On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > output queue resources. Call synchronize_net() to avoid such races.
> >
> > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > ---
> > V2:
> >   - Changed sync mechanism to fix race conditions from using an atomic
> >     set_bit ops to a much simpler synchronize_net()
> >
> > V1: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lore.kernel.org_all_20241203072130.2316913-2D2-2Dsrasheed-
> 40marvell.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-
> oxrlgQ1rjXgWtmLz1pnaDjD96sDq-
> cKUwK4&m=SfdHxslanlPmhVsD4m3qSuUI2eout0Tqi9o-
> DnPbfjVzv5Nte11HnOw2-aIeEN3i&s=0iv3vwi5W-
> le8dxI6EtazU7q4RlnvElcVk5MsyMm_0k&e=
> >
> >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > index 549436efc204..941bbaaa67b5 100644
> > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device *netdev)
> >  {
> >  	struct octep_device *oct = netdev_priv(netdev);
> >
> > +	synchronize_net();
> 
> You should have elaborated on the fact that this synchronize_net() is for
> __LINK_STATE_START flag in the commit message, this is not obvious. Also, is
> octep_get_stats64() called from RCU-safe context?

The octep_get_stats64() is an instance of the ndo_get_stats64() netdev op. As I understand, the netdev op is supposed to be
called from RCU-safe contexts. Please do correct me if I'm wrong.

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

[Shinas Rasheed]

Hi Larysa,

> On Mon, Dec 16, 2024 at 03:30:12PM +0100, Larysa Zaremba wrote:
> > On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > > output queue resources. Call synchronize_net() to avoid such races.
> > >
> > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > > ---
> > > V2:
> > >   - Changed sync mechanism to fix race conditions from using an atomic
> > >     set_bit ops to a much simpler synchronize_net()
> > >
> > > V1: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lore.kernel.org_all_20241203072130.2316913-2D2-2Dsrasheed-
> 40marvell.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-
> oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=Dh7BH5wsuCdQnE-
> 4erjptaJnM42YsLU2tY4wPn5NWqwsymkNOllPfQAkomj1mXPN&s=IjWHk3SOqr
> ibgv6kz-WTL8VfGVInSu5DzKSbcjCFIvk&e=
> > >
> > >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > index 549436efc204..941bbaaa67b5 100644
> > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device *netdev)
> > >  {
> > >  	struct octep_device *oct = netdev_priv(netdev);
> > >
> > > +	synchronize_net();
> >
> > You should have elaborated on the fact that this synchronize_net() is for
> > __LINK_STATE_START flag in the commit message, this is not obvious. Also,
> is
> > octep_get_stats64() called from RCU-safe context?
> >
> 
> Now I see that in case !netif_running(), you do not bail out of
> octep_get_stats64() fully (or at all after the second patch). So, could you
> explain, how are you utilizing RCU here?
> 

The understanding is that octep_get_stats64() (.ndo_get_stats64() in turn) is called from RCU safe contexts, and
that the netdev op is never called after the ndo_stop().

Thanks for the comments

RE: [EXTERNAL] Re: [PATCH net v2 2/4] octeon_ep: remove firmware stats fetch in ndo_get_stats64

[Shinas Rasheed]

Hi Larysa,


> On Sun, Dec 15, 2024 at 11:58:40PM -0800, Shinas Rasheed wrote:
> > 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>
> > @@ -1019,10 +1013,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;
> 
> I do not see, how it is a fix to remove some fields from stats. If this is a
> cleanup, it should not go to the stable tree.
> 
> >  }
> >

The fix part of this patch is to remove the call to firmware to retrieve stats, which could block and cause rcu read lock warnings.
The fields that are retrieved by this stats call can be neglected for use cases concerning the DPU, and the necessary stats are already
read from per queue hardware stats registers. Hence, why the code is removed.

Re: [EXTERNAL] Re: [PATCH net v2 2/4] octeon_ep: remove firmware stats fetch in ndo_get_stats64

[Larysa Zaremba]

On Mon, Dec 16, 2024 at 06:31:05PM +0000, Shinas Rasheed wrote:
> Hi Larysa,
> 
> 
> > On Sun, Dec 15, 2024 at 11:58:40PM -0800, Shinas Rasheed wrote:
> > > 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>
> > > @@ -1019,10 +1013,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;
> > 
> > I do not see, how it is a fix to remove some fields from stats. If this is a
> > cleanup, it should not go to the stable tree.
> > 
> > >  }
> > >
> 
> The fix part of this patch is to remove the call to firmware to retrieve stats, which could block and cause rcu read lock warnings.
> The fields that are retrieved by this stats call can be neglected for use cases concerning the DPU, and the necessary stats are already
> read from per queue hardware stats registers. Hence, why the code is removed.
>

Please, provide the warinings in question in the commit message, this is an 
important context for anyone that would need to look through code history. 

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

[Larysa Zaremba]

On Mon, Dec 16, 2024 at 06:28:13PM +0000, Shinas Rasheed wrote:
> Hi Larysa,
> 
> > On Mon, Dec 16, 2024 at 03:30:12PM +0100, Larysa Zaremba wrote:
> > > On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > > > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > > > output queue resources. Call synchronize_net() to avoid such races.
> > > >
> > > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > > > ---
> > > > V2:
> > > >   - Changed sync mechanism to fix race conditions from using an atomic
> > > >     set_bit ops to a much simpler synchronize_net()
> > > >
> > > > V1: https://urldefense.proofpoint.com/v2/url?u=https-
> > 3A__lore.kernel.org_all_20241203072130.2316913-2D2-2Dsrasheed-
> > 40marvell.com_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=1OxLD4y-
> > oxrlgQ1rjXgWtmLz1pnaDjD96sDq-cKUwK4&m=Dh7BH5wsuCdQnE-
> > 4erjptaJnM42YsLU2tY4wPn5NWqwsymkNOllPfQAkomj1mXPN&s=IjWHk3SOqr
> > ibgv6kz-WTL8VfGVInSu5DzKSbcjCFIvk&e=
> > > >
> > > >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > index 549436efc204..941bbaaa67b5 100644
> > > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device *netdev)
> > > >  {
> > > >  	struct octep_device *oct = netdev_priv(netdev);
> > > >
> > > > +	synchronize_net();
> > >
> > > You should have elaborated on the fact that this synchronize_net() is for
> > > __LINK_STATE_START flag in the commit message, this is not obvious. Also,
> > is
> > > octep_get_stats64() called from RCU-safe context?
> > >
> > 
> > Now I see that in case !netif_running(), you do not bail out of
> > octep_get_stats64() fully (or at all after the second patch). So, could you
> > explain, how are you utilizing RCU here?
> > 
> 
> The understanding is that octep_get_stats64() (.ndo_get_stats64() in turn) is called from RCU safe contexts, and
> that the netdev op is never called after the ndo_stop().

As I now see, in net/core/net-sysfs.c, yes there is an rcu read lock around the 
thing, but there are a lot more callers and for example veth_get_stats64() 
explicitly calls rcu_read_lock().

Also, even with RCU-protected section, I am not sure prevents the 
octep_get_stats64() to be called after synchronize_net() finishes. Again, the 
callers seem too diverse to definitely say that we can rely on built-in flags 
for this to not happen :/

> 
> Thanks for the comments

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

[Shinas Rasheed]

> > > On Mon, Dec 16, 2024 at 03:30:12PM +0100, Larysa Zaremba wrote:
> > > > On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > > > > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > > > > output queue resources. Call synchronize_net() to avoid such races.
> > > > >
> > > > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > > > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > > > > ---
> > > > > V2:
> > > > >   - Changed sync mechanism to fix race conditions from using an
> atomic
> > > > >     set_bit ops to a much simpler synchronize_net()
> > > > >
> > > > >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > index 549436efc204..941bbaaa67b5 100644
> > > > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device
> *netdev)
> > > > >  {
> > > > >  	struct octep_device *oct = netdev_priv(netdev);
> > > > >
> > > > > +	synchronize_net();
> > > >
> > > > You should have elaborated on the fact that this synchronize_net() is for
> > > > __LINK_STATE_START flag in the commit message, this is not obvious.
> Also,
> > > is
> > > > octep_get_stats64() called from RCU-safe context?
> > > >
> > >
> > > Now I see that in case !netif_running(), you do not bail out of
> > > octep_get_stats64() fully (or at all after the second patch). So, could you
> > > explain, how are you utilizing RCU here?
> > >
> >
> > The understanding is that octep_get_stats64() (.ndo_get_stats64() in turn) is
> called from RCU safe contexts, and
> > that the netdev op is never called after the ndo_stop().
> 
> As I now see, in net/core/net-sysfs.c, yes there is an rcu read lock around the
> thing, but there are a lot more callers and for example veth_get_stats64()
> explicitly calls rcu_read_lock().
> 
> Also, even with RCU-protected section, I am not sure prevents the
> octep_get_stats64() to be called after synchronize_net() finishes. Again, the
> callers seem too diverse to definitely say that we can rely on built-in flags
> for this to not happen :/

Usually, the understanding is that ndo_get_stats won't be called by the network stack after the interface is put down. As long as that is the case, I don't think we should keep adding checks until there is a strong reason to do so. What do you think?

> > Thanks for the comments

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

[Larysa Zaremba]

On Tue, Dec 17, 2024 at 05:49:13PM +0000, Shinas Rasheed wrote:
> 
> 
> > > > On Mon, Dec 16, 2024 at 03:30:12PM +0100, Larysa Zaremba wrote:
> > > > > On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > > > > > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > > > > > output queue resources. Call synchronize_net() to avoid such races.
> > > > > >
> > > > > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > > > > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > > > > > ---
> > > > > > V2:
> > > > > >   - Changed sync mechanism to fix race conditions from using an
> > atomic
> > > > > >     set_bit ops to a much simpler synchronize_net()
> > > > > >
> > > > > >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > > index 549436efc204..941bbaaa67b5 100644
> > > > > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device
> > *netdev)
> > > > > >  {
> > > > > >  	struct octep_device *oct = netdev_priv(netdev);
> > > > > >
> > > > > > +	synchronize_net();
> > > > >
> > > > > You should have elaborated on the fact that this synchronize_net() is for
> > > > > __LINK_STATE_START flag in the commit message, this is not obvious.
> > Also,
> > > > is
> > > > > octep_get_stats64() called from RCU-safe context?
> > > > >
> > > >
> > > > Now I see that in case !netif_running(), you do not bail out of
> > > > octep_get_stats64() fully (or at all after the second patch). So, could you
> > > > explain, how are you utilizing RCU here?
> > > >
> > >
> > > The understanding is that octep_get_stats64() (.ndo_get_stats64() in turn) is
> > called from RCU safe contexts, and
> > > that the netdev op is never called after the ndo_stop().
> > 
> > As I now see, in net/core/net-sysfs.c, yes there is an rcu read lock around the
> > thing, but there are a lot more callers and for example veth_get_stats64()
> > explicitly calls rcu_read_lock().
> > 
> > Also, even with RCU-protected section, I am not sure prevents the
> > octep_get_stats64() to be called after synchronize_net() finishes. Again, the
> > callers seem too diverse to definitely say that we can rely on built-in flags
> > for this to not happen :/
> 
> Usually, the understanding is that ndo_get_stats won't be called by the network stack after the interface is put down. As long as that is the case, I don't think we should keep adding checks until there is a strong reason to do so. What do you think?
>

It is hard to know without testing (but testing should not be hard). I think the 
phrase "Statistics must persist across routine operations like bringing the 
interface down and up." [0] implies that bringing the interface down may not 
necessarily prevent stats calls.

[0] https://docs.kernel.org/networking/statistics.html
 
> > > Thanks for the comments

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

[Shinas Rasheed]

Hi Larysa,

> > > > > On Mon, Dec 16, 2024 at 03:30:12PM +0100, Larysa Zaremba wrote:
> > > > > > On Sun, Dec 15, 2024 at 11:58:39PM -0800, Shinas Rasheed wrote:
> > > > > > > ndo_get_stats64() can race with ndo_stop(), which frees input and
> > > > > > > output queue resources. Call synchronize_net() to avoid such
> races.
> > > > > > >
> > > > > > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops")
> > > > > > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> > > > > > > ---
> > > > > > > V2:
> > > > > > >   - Changed sync mechanism to fix race conditions from using an
> > > atomic
> > > > > > >     set_bit ops to a much simpler synchronize_net()
> > > > > > >
> > > > > > >  drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > > > index 549436efc204..941bbaaa67b5 100644
> > > > > > > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > > > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> > > > > > > @@ -757,6 +757,7 @@ static int octep_stop(struct net_device
> > > *netdev)
> > > > > > >  {
> > > > > > >  	struct octep_device *oct = netdev_priv(netdev);
> > > > > > >
> > > > > > > +	synchronize_net();
> > > > > >
> > > > > > You should have elaborated on the fact that this synchronize_net() is
> for
> > > > > > __LINK_STATE_START flag in the commit message, this is not obvious.
> > > Also,
> > > > > is
> > > > > > octep_get_stats64() called from RCU-safe context?
> > > > > >
> > > > >
> > > > > Now I see that in case !netif_running(), you do not bail out of
> > > > > octep_get_stats64() fully (or at all after the second patch). So, could
> you
> > > > > explain, how are you utilizing RCU here?
> > > > >
> > > >
> > > > The understanding is that octep_get_stats64() (.ndo_get_stats64() in
> turn) is
> > > called from RCU safe contexts, and
> > > > that the netdev op is never called after the ndo_stop().
> > >
> > > As I now see, in net/core/net-sysfs.c, yes there is an rcu read lock around
> the
> > > thing, but there are a lot more callers and for example veth_get_stats64()
> > > explicitly calls rcu_read_lock().
> > >
> > > Also, even with RCU-protected section, I am not sure prevents the
> > > octep_get_stats64() to be called after synchronize_net() finishes. Again,
> the
> > > callers seem too diverse to definitely say that we can rely on built-in flags
> > > for this to not happen :/
> >
> > Usually, the understanding is that ndo_get_stats won't be called by the
> network stack after the interface is put down. As long as that is the case, I
> don't think we should keep adding checks until there is a strong reason to do
> so. What do you think?
> >
> 
> It is hard to know without testing (but testing should not be hard). I think the
> phrase "Statistics must persist across routine operations like bringing the
> interface down and up." [0] implies that bringing the interface down may not
> necessarily prevent stats calls.
> 
> [0] https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__docs.kernel.org_networking_statistics.html&d=DwIBAg&c=nKjWec2b6R0
> mOyPaz7xtfQ&r=1OxLD4y-oxrlgQ1rjXgWtmLz1pnaDjD96sDq-
> cKUwK4&m=DJzJNo9WT10pSHikJhCBbN7-CfB-
> O2kz9OVGsmiIRQXvcIIWDK6034tMmzZGvlFs&s=essE01suLWF42taNi0yJ3H3YC
> 0Et8GofMj5wxor9yD4&e=
>

Sorry, I misworded my previous statement. Of course ndo_get_stats can get called while the netdev is down. This is tested code, and the reason why there is no issue in this scenario is because for a ndo_get_stats call that happens after ndo_stop() happens, the oct->num_oqs will be seen as 0, and hence octep_iq nor octep_oq is not accessed in the for loop that follows. Octep_iq and octep_oq are the resources that we're trying to protect from race conditions. Hope that clarifies things.


> > > > Thanks for the comments

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

[Eric Dumazet]

On Wed, Dec 18, 2024 at 2:25 PM Larysa Zaremba <larysa.zaremba@intel.com> wrote:

>
> It is hard to know without testing (but testing should not be hard). I think the
> phrase "Statistics must persist across routine operations like bringing the
> interface down and up." [0] implies that bringing the interface down may not
> necessarily prevent stats calls.

Please don't  add workarounds to individual drivers.

I think the core networking stack should handle the possible races.

Most dev_get_stats() callers are correctly testing dev_isalive() or
are protected by RTNL.

There are few nested cases that are not properly handled, the
following patch should take care of them.


diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2593019ad5b1614f3b8c037afb4ba4fa740c7d51..768afc2a18d343d051e7a1b631124910af9922d2
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -5342,6 +5342,12 @@ static inline const char
*netdev_reg_state(const struct net_device *dev)
        return " (unknown)";
 }

+/* Caller holds RTNL or RCU */
+static inline int dev_isalive(const struct net_device *dev)
+{
+       return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
+}
+
 #define MODULE_ALIAS_NETDEV(device) \
        MODULE_ALIAS("netdev-" device)

diff --git a/net/core/dev.c b/net/core/dev.c
index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..f11f305f3136f208fcb285c7b314914aef20dfad
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -11044,8 +11044,13 @@ struct rtnl_link_stats64
*dev_get_stats(struct net_device *dev,
        const struct net_device_ops *ops = dev->netdev_ops;
        const struct net_device_core_stats __percpu *p;

+       memset(storage, 0, sizeof(*storage));
+       rcu_read_lock();
+
+       if (unlikely(!dev_isalive(dev)))
+               goto unlock;
+
        if (ops->ndo_get_stats64) {
-               memset(storage, 0, sizeof(*storage));
                ops->ndo_get_stats64(dev, storage);
        } else if (ops->ndo_get_stats) {
                netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
@@ -11071,6 +11076,8 @@ struct rtnl_link_stats64 *dev_get_stats(struct
net_device *dev,
                        storage->rx_otherhost_dropped +=
READ_ONCE(core_stats->rx_otherhost_dropped);
                }
        }
+unlock:
+       rcu_read_unlock();
        return storage;
 }
 EXPORT_SYMBOL(dev_get_stats);
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 2d9afc6e2161efa51ffa62813ec10c8f43944bce..3f4851d67015c959dd531c571c46fc2ac18beb65
100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -36,12 +36,6 @@ static const char fmt_uint[] = "%u\n";
 static const char fmt_ulong[] = "%lu\n";
 static const char fmt_u64[] = "%llu\n";

-/* Caller holds RTNL or RCU */
-static inline int dev_isalive(const struct net_device *dev)
-{
-       return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
-}
-
 /* use same locking rules as GIF* ioctl's */
 static ssize_t netdev_show(const struct device *dev,
                           struct device_attribute *attr, char *buf,

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

[Larysa Zaremba]

On Wed, Dec 18, 2024 at 03:21:12PM +0100, Eric Dumazet wrote:
> On Wed, Dec 18, 2024 at 2:25 PM Larysa Zaremba <larysa.zaremba@intel.com> wrote:
> 
> >
> > It is hard to know without testing (but testing should not be hard). I think the
> > phrase "Statistics must persist across routine operations like bringing the
> > interface down and up." [0] implies that bringing the interface down may not
> > necessarily prevent stats calls.
> 
> Please don't  add workarounds to individual drivers.
> 
> I think the core networking stack should handle the possible races.
> 
> Most dev_get_stats() callers are correctly testing dev_isalive() or
> are protected by RTNL.
> 
> There are few nested cases that are not properly handled, the
> following patch should take care of them.
>

I was under the impression that .ndo_stop() being called does not mean the 
device stops being NETREG_REGISTERED, such link would be required to solve the 
original problem  with your patch alone (though it is generally a good change).
Could you please explain this relation?

> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2593019ad5b1614f3b8c037afb4ba4fa740c7d51..768afc2a18d343d051e7a1b631124910af9922d2
> 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -5342,6 +5342,12 @@ static inline const char
> *netdev_reg_state(const struct net_device *dev)
>         return " (unknown)";
>  }
> 
> +/* Caller holds RTNL or RCU */
> +static inline int dev_isalive(const struct net_device *dev)
> +{
> +       return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
> +}
> +
>  #define MODULE_ALIAS_NETDEV(device) \
>         MODULE_ALIAS("netdev-" device)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..f11f305f3136f208fcb285c7b314914aef20dfad
> 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -11044,8 +11044,13 @@ struct rtnl_link_stats64
> *dev_get_stats(struct net_device *dev,
>         const struct net_device_ops *ops = dev->netdev_ops;
>         const struct net_device_core_stats __percpu *p;
> 
> +       memset(storage, 0, sizeof(*storage));
> +       rcu_read_lock();
> +
> +       if (unlikely(!dev_isalive(dev)))
> +               goto unlock;
> +
>         if (ops->ndo_get_stats64) {
> -               memset(storage, 0, sizeof(*storage));
>                 ops->ndo_get_stats64(dev, storage);
>         } else if (ops->ndo_get_stats) {
>                 netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
> @@ -11071,6 +11076,8 @@ struct rtnl_link_stats64 *dev_get_stats(struct
> net_device *dev,
>                         storage->rx_otherhost_dropped +=
> READ_ONCE(core_stats->rx_otherhost_dropped);
>                 }
>         }
> +unlock:
> +       rcu_read_unlock();
>         return storage;
>  }
>  EXPORT_SYMBOL(dev_get_stats);
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 2d9afc6e2161efa51ffa62813ec10c8f43944bce..3f4851d67015c959dd531c571c46fc2ac18beb65
> 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -36,12 +36,6 @@ static const char fmt_uint[] = "%u\n";
>  static const char fmt_ulong[] = "%lu\n";
>  static const char fmt_u64[] = "%llu\n";
> 
> -/* Caller holds RTNL or RCU */
> -static inline int dev_isalive(const struct net_device *dev)
> -{
> -       return READ_ONCE(dev->reg_state) <= NETREG_REGISTERED;
> -}
> -
>  /* use same locking rules as GIF* ioctl's */
>  static ssize_t netdev_show(const struct device *dev,
>                            struct device_attribute *attr, char *buf,

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

[Eric Dumazet]

On Wed, Dec 18, 2024 at 3:49 PM Larysa Zaremba <larysa.zaremba@intel.com> wrote:
>
> On Wed, Dec 18, 2024 at 03:21:12PM +0100, Eric Dumazet wrote:
> > On Wed, Dec 18, 2024 at 2:25 PM Larysa Zaremba <larysa.zaremba@intel.com> wrote:
> >
> > >
> > > It is hard to know without testing (but testing should not be hard). I think the
> > > phrase "Statistics must persist across routine operations like bringing the
> > > interface down and up." [0] implies that bringing the interface down may not
> > > necessarily prevent stats calls.
> >
> > Please don't  add workarounds to individual drivers.
> >
> > I think the core networking stack should handle the possible races.
> >
> > Most dev_get_stats() callers are correctly testing dev_isalive() or
> > are protected by RTNL.
> >
> > There are few nested cases that are not properly handled, the
> > following patch should take care of them.
> >
>
> I was under the impression that .ndo_stop() being called does not mean the
> device stops being NETREG_REGISTERED, such link would be required to solve the
> original problem  with your patch alone (though it is generally a good change).
> Could you please explain this relation?
>

ndo_stop() being called must have no impact on statistics :

# ip -s link sh dev lo
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN
mode DEFAULT group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    RX:  bytes packets errors dropped  missed   mcast
       3473568   41352      0       0       0       0
    TX:  bytes packets errors dropped carrier collsns
       3473568   41352      0       0       0       0

# ip link set dev lo down  # would call ndo_stop() if loopback had one

# ip -s link sh dev lo
1: lo: <LOOPBACK> mtu 65536 qdisc noqueue state DOWN mode DEFAULT
group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    RX:  bytes packets errors dropped  missed   mcast
       3473568   41352      0       0       0       0
    TX:  bytes packets errors dropped carrier collsns
       3473568   41352      0       0       0       0


So perhaps the problem with this driver is that its ndo_stop() is
doing things it should not.

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

[Shinas Rasheed]

> > On Wed, Dec 18, 2024 at 03:21:12PM +0100, Eric Dumazet wrote:
> > > On Wed, Dec 18, 2024 at 2:25 PM Larysa Zaremba
> <larysa.zaremba@intel.com> wrote:
> > >
> > > >
> > > > It is hard to know without testing (but testing should not be hard). I
> think the
> > > > phrase "Statistics must persist across routine operations like bringing the
> > > > interface down and up." [0] implies that bringing the interface down
> may not
> > > > necessarily prevent stats calls.
> > >
> > > Please don't  add workarounds to individual drivers.
> > >
> > > I think the core networking stack should handle the possible races.
> > >
> > > Most dev_get_stats() callers are correctly testing dev_isalive() or
> > > are protected by RTNL.
> > >
> > > There are few nested cases that are not properly handled, the
> > > following patch should take care of them.
> > >
> >
> > I was under the impression that .ndo_stop() being called does not mean the
> > device stops being NETREG_REGISTERED, such link would be required to
> solve the
> > original problem  with your patch alone (though it is generally a good
> change).
> > Could you please explain this relation?
> >
> 
> ndo_stop() being called must have no impact on statistics :
> 
> # ip -s link sh dev lo
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN
> mode DEFAULT group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>     RX:  bytes packets errors dropped  missed   mcast
>        3473568   41352      0       0       0       0
>     TX:  bytes packets errors dropped carrier collsns
>        3473568   41352      0       0       0       0
> 
> # ip link set dev lo down  # would call ndo_stop() if loopback had one
> 
> # ip -s link sh dev lo
> 1: lo: <LOOPBACK> mtu 65536 qdisc noqueue state DOWN mode DEFAULT
> group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>     RX:  bytes packets errors dropped  missed   mcast
>        3473568   41352      0       0       0       0
>     TX:  bytes packets errors dropped carrier collsns
>        3473568   41352      0       0       0       0
> 
> 
> So perhaps the problem with this driver is that its ndo_stop() is
> doing things it should not.

Hi Eric,

This patch is not a workaround. In some setups, we were seeing races with regards
to resource freeing between ndo_stop() and ndo_get_stats(). Hence to sync with the view of
resources, a synchronize_net() is called in ndo_stop(). Please let me know if you see anything wrong here.

Thanks

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

[Eric Dumazet]

On Wed, Dec 18, 2024 at 4:25 PM Shinas Rasheed <srasheed@marvell.com> wrote:

> Hi Eric,
>
> This patch is not a workaround. In some setups, we were seeing races with regards
> to resource freeing between ndo_stop() and ndo_get_stats(). Hence to sync with the view of
> resources, a synchronize_net() is called in ndo_stop(). Please let me know if you see anything wrong here.

We do not add a synchronize_net() without a very strong explanation
(details, not a weak sentence in the changelog).

Where is the opposite barrier in your patch ?

I am saying you do not need this, unless you can show evidence.

If your ndo_get_stats() needs to call netif_running(), this would be
the fix IMO.

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

[Shinas Rasheed]

Hi Eric,

> On Wed, Dec 18, 2024 at 4:25 PM Shinas Rasheed <srasheed@marvell.com>
> wrote:
> 
> > Hi Eric,
> >
> > This patch is not a workaround. In some setups, we were seeing races with
> regards
> > to resource freeing between ndo_stop() and ndo_get_stats(). Hence to sync
> with the view of
> > resources, a synchronize_net() is called in ndo_stop(). Please let me know if
> you see anything wrong here.
> 
> We do not add a synchronize_net() without a very strong explanation
> (details, not a weak sentence in the changelog).
> 
> Where is the opposite barrier in your patch ?
> 
> I am saying you do not need this, unless you can show evidence.
> 
> If your ndo_get_stats() needs to call netif_running(), this would be
> the fix IMO.

The synchronize_net() is supposed to sync all previous calls of ndo_get_stats() and wait for their completion before closing the device.
Again this seems to be the v2 of this patch. In the v3, I have provided the warn log as well in the commit message for reference, in answer to
the changelog comment for more clarification.

As I stated, this is needed because ndo_stop() races with ndo_get_stats(), and a 'lock' or a similar mechanism seems required to alleviate this.
Fixes in the same vein seem to be common, as I do see other drivers utilizing a lock mechanism while retrieving statistics to resolve the same 
(ie; race with resource destruction in ndo_stop()). So, just to state, I'm not trying to do anything new
here.

Can we please comment further on the v3 of this patch? https://lore.kernel.org/all/20241218115111.2407958-1-srasheed@marvell.com/

Thanks a lot for your time and comments

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

[Eric Dumazet]

On Thu, Dec 19, 2024 at 5:28 PM Shinas Rasheed <srasheed@marvell.com> wrote:
>
> Hi Eric,
>
> > On Wed, Dec 18, 2024 at 4:25 PM Shinas Rasheed <srasheed@marvell.com>
> > wrote:
> >
> > > Hi Eric,
> > >
> > > This patch is not a workaround. In some setups, we were seeing races with
> > regards
> > > to resource freeing between ndo_stop() and ndo_get_stats(). Hence to sync
> > with the view of
> > > resources, a synchronize_net() is called in ndo_stop(). Please let me know if
> > you see anything wrong here.
> >
> > We do not add a synchronize_net() without a very strong explanation
> > (details, not a weak sentence in the changelog).
> >
> > Where is the opposite barrier in your patch ?
> >
> > I am saying you do not need this, unless you can show evidence.
> >
> > If your ndo_get_stats() needs to call netif_running(), this would be
> > the fix IMO.
>
> The synchronize_net() is supposed to sync all previous calls of ndo_get_stats() and wait for their completion before closing the device.
> Again this seems to be the v2 of this patch. In the v3, I have provided the warn log as well in the commit message for reference, in answer to
> the changelog comment for more clarification.
>
> As I stated, this is needed because ndo_stop() races with ndo_get_stats(), and a 'lock' or a similar mechanism seems required to alleviate this.
> Fixes in the same vein seem to be common, as I do see other drivers utilizing a lock mechanism while retrieving statistics to resolve the same
> (ie; race with resource destruction in ndo_stop()). So, just to state, I'm not trying to do anything new
> here.

A precise lock is better, it is easy to grep, if really other drivers
were not able to fix this in a different way.

A synchronize_net() without explicit details is very very weak.

>
> Can we please comment further on the v3 of this patch? https://lore.kernel.org/all/20241218115111.2407958-1-srasheed@marvell.com/
>
> Thanks a lot for your time and comments

I tried to say that adding a synchronize_net() call in ndo_stop() was
not needed.

I do not particularly care for this driver, my concern is that
copy/pasting is going to happen.