[PATCH net] net: sched: gred: prevent races when adding offloads to stats

[PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Jakub Kicinski]

Naresh reports seeing a warning that gred is calling
u64_stats_update_begin() with preemption enabled.
Arnd points out it's coming from _bstats_update().

We should be holding the qdisc lock when writing
to stats, they are also updated from the datapath.

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Link: https://lore.kernel.org/all/CA+G9fYsTr9_r893+62u6UGD3dVaCE-kN9C-Apmb2m=hxjc1Cqg@mail.gmail.com/
Fixes: e49efd5288bd ("net: sched: gred: support reporting stats from offloads")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jhs@mojatatu.com
CC: xiyou.wangcong@gmail.com
CC: jiri@resnulli.us
CC: john.hurley@netronome.com
---
 net/sched/sch_gred.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index a661b062cca8..872d127c9db4 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -377,6 +377,7 @@ static int gred_offload_dump_stats(struct Qdisc *sch)
 	/* Even if driver returns failure adjust the stats - in case offload
 	 * ended but driver still wants to adjust the values.
 	 */
+	sch_tree_lock(sch);
 	for (i = 0; i < MAX_DPs; i++) {
 		if (!table->tab[i])
 			continue;
@@ -393,6 +394,7 @@ static int gred_offload_dump_stats(struct Qdisc *sch)
 		sch->qstats.overlimits += hw_stats->stats.qstats[i].overlimits;
 	}
 	_bstats_update(&sch->bstats, bytes, packets);
+	sch_tree_unlock(sch);
 
 	kfree(hw_stats);
 	return ret;
-- 
2.38.1

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Cong Wang]

On Thu, Jan 12, 2023 at 08:41:37PM -0800, Jakub Kicinski wrote:
> Naresh reports seeing a warning that gred is calling
> u64_stats_update_begin() with preemption enabled.
> Arnd points out it's coming from _bstats_update().


The stack trace looks confusing to me without further decoding.

Are you sure we use sch->qstats/bstats in __dev_queue_xmit() there
not any netdev stats? It may be a false positive one as they may end up
with the same lockdep class.

Thanks.

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Paolo Abeni]

Hello,

On Sat, 2023-01-14 at 18:20 -0800, Cong Wang wrote:
> On Thu, Jan 12, 2023 at 08:41:37PM -0800, Jakub Kicinski wrote:
> > Naresh reports seeing a warning that gred is calling
> > u64_stats_update_begin() with preemption enabled.
> > Arnd points out it's coming from _bstats_update().
> 
> 
> The stack trace looks confusing to me without further decoding.
> 
> Are you sure we use sch->qstats/bstats in __dev_queue_xmit() there
> not any netdev stats? It may be a false positive one as they may end up
> with the same lockdep class.

I'm unsure I read you comment correctly. Please note that the
referenced message includes several splats. The first one - arguably
the most relevant - points to the lack of locking in the gred control
path.

The posted patch LGTM, could you please re-phrase your doubts?

Thanks,

Paolo

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Jakub Kicinski]

On Tue, 17 Jan 2023 10:00:56 +0100 Paolo Abeni wrote:
> On Sat, 2023-01-14 at 18:20 -0800, Cong Wang wrote:
> > On Thu, Jan 12, 2023 at 08:41:37PM -0800, Jakub Kicinski wrote:  
> > > Naresh reports seeing a warning that gred is calling
> > > u64_stats_update_begin() with preemption enabled.
> > > Arnd points out it's coming from _bstats_update().  
> > 
> > The stack trace looks confusing to me without further decoding.
> > 
> > Are you sure we use sch->qstats/bstats in __dev_queue_xmit() there
> > not any netdev stats? It may be a false positive one as they may end up
> > with the same lockdep class.  

I didn't repro this myself, TBH, but there is u64_stats_update_begin() 
inside _bstats_update(). Pretty sure it will trigger the warning that
preemption is not disabled on non-SMP systems.

> I'm unsure I read you comment correctly. Please note that the
> referenced message includes several splats. The first one - arguably
> the most relevant - points to the lack of locking in the gred control
> path.

Yup, I'm not really sure if we're fixing the right splat for the bug.
But I am fairly confident we should be holding a lock while writing
bstats from the dump path, enqueue/dequeue may run concurrently.

> The posted patch LGTM, could you please re-phrase your doubts?

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 12 Jan 2023 20:41:37 -0800 you wrote:
> Naresh reports seeing a warning that gred is calling
> u64_stats_update_begin() with preemption enabled.
> Arnd points out it's coming from _bstats_update().
> 
> We should be holding the qdisc lock when writing
> to stats, they are also updated from the datapath.
> 
> [...]

Here is the summary with links:
  - [net] net: sched: gred: prevent races when adding offloads to stats
    https://git.kernel.org/netdev/net/c/339346d49ae0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Cong Wang]

On Tue, Jan 17, 2023 at 10:00:56AM +0100, Paolo Abeni wrote:
> Hello,
> 
> On Sat, 2023-01-14 at 18:20 -0800, Cong Wang wrote:
> > On Thu, Jan 12, 2023 at 08:41:37PM -0800, Jakub Kicinski wrote:
> > > Naresh reports seeing a warning that gred is calling
> > > u64_stats_update_begin() with preemption enabled.
> > > Arnd points out it's coming from _bstats_update().
> > 
> > 
> > The stack trace looks confusing to me without further decoding.
> > 
> > Are you sure we use sch->qstats/bstats in __dev_queue_xmit() there
> > not any netdev stats? It may be a false positive one as they may end up
> > with the same lockdep class.
> 
> I'm unsure I read you comment correctly. Please note that the
> referenced message includes several splats. The first one - arguably
> the most relevant - points to the lack of locking in the gred control
> path.

I think it means lack of BH disable, not necessarily locking.

> 
> The posted patch LGTM, could you please re-phrase your doubts?

Yes, because the key in lockdep ("&syncp->seq#14") may be same for other
dev stats (aka, not qdisc stats).

Also, HTB does not accquire this lock when dumping
(htb_dump_class_stats()) stats. Why don't we have a problem there? ;)

Thanks.

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Cong Wang]

On Tue, Jan 17, 2023 at 11:10:19AM -0800, Jakub Kicinski wrote:
> On Tue, 17 Jan 2023 10:00:56 +0100 Paolo Abeni wrote:
> > On Sat, 2023-01-14 at 18:20 -0800, Cong Wang wrote:
> > > On Thu, Jan 12, 2023 at 08:41:37PM -0800, Jakub Kicinski wrote:  
> > > > Naresh reports seeing a warning that gred is calling
> > > > u64_stats_update_begin() with preemption enabled.
> > > > Arnd points out it's coming from _bstats_update().  
> > > 
> > > The stack trace looks confusing to me without further decoding.
> > > 
> > > Are you sure we use sch->qstats/bstats in __dev_queue_xmit() there
> > > not any netdev stats? It may be a false positive one as they may end up
> > > with the same lockdep class.  
> 
> I didn't repro this myself, TBH, but there is u64_stats_update_begin() 
> inside _bstats_update(). Pretty sure it will trigger the warning that
> preemption is not disabled on non-SMP systems.
> 
> > I'm unsure I read you comment correctly. Please note that the
> > referenced message includes several splats. The first one - arguably
> > the most relevant - points to the lack of locking in the gred control
> > path.
> 
> Yup, I'm not really sure if we're fixing the right splat for the bug.
> But I am fairly confident we should be holding a lock while writing
> bstats from the dump path, enqueue/dequeue may run concurrently.

Explain htb_dump_class_stats()? :) I see two _bstats_update() calls but
I don't see any tree lock there.

Thanks.

Re: [PATCH net] net: sched: gred: prevent races when adding offloads to stats

[Jakub Kicinski]

On Thu, 19 Jan 2023 12:19:53 -0800 Cong Wang wrote:
> > > I'm unsure I read you comment correctly. Please note that the
> > > referenced message includes several splats. The first one - arguably
> > > the most relevant - points to the lack of locking in the gred control
> > > path.  
> > 
> > Yup, I'm not really sure if we're fixing the right splat for the bug.
> > But I am fairly confident we should be holding a lock while writing
> > bstats from the dump path, enqueue/dequeue may run concurrently.  
> 
> Explain htb_dump_class_stats()? :) I see two _bstats_update() calls but
> I don't see any tree lock there.

I don't know the HTB offload, if the datapath writes to the same
cl->bstats then it's buggy in the same way. But maybe it doesn't - 
the HTB offload is local (as in offloads normal host egress) while
the RED offloads are for switches so the data flows both thru the
"representor" netdevs and therefore the SW instance and the offload 
on the HW forwarding path.