[PATCH net] net/sched: gred: keep the qdisc lock while updating the table

[PATCH net] net/sched: gred: keep the qdisc lock while updating the table

[Dudu Lu]

gred_change_table_def() publishes table->DPs/def/red_flags under
sch_tree_lock(), drops the lock, and then keeps writing state the
datapath reads without it:

  - gred_enable_rio_mode()/gred_disable_rio_mode()/gred_disable_wred_mode()
    set/clear table->flags;
  - the red_flags_changed loop writes table->tab[i]->red_flags;
  - the shadowed-VQ loop frees table->tab[i] and clears the slot.

gred_enqueue() and gred_dequeue() read those same fields (test_bit on
table->flags, q->red_flags, t->tab[]/t->DPs) under the same root lock on
another CPU, so the post-unlock writes race with the enqueue path:

  BUG: KCSAN: data-race in gred_change_table_def [sch_gred] / gred_enqueue [sch_gred]
  read (marked) to 0xffff000007ad3b00 of 8 bytes by task 110 on cpu 0:
   gred_enqueue+0x88/0xc60 [sch_gred]
   dev_qdisc_enqueue+0x4c/0x19c
   __dev_queue_xmit+0x10a4/0x1ea4

The comment justifying the early unlock only covers the DP-vs-DPs check,
which the shadowed-VQ free loop relies on; it does not cover the scalar
mode bits and per-VQ red_flags written afterwards.  Move the unlock below
those updates so everything the datapath reads is published under the
lock that protects it.

Fixes: 6639607ed9de ("[PKT_SCHED]: GRED: Use a central table definition change procedure")
Assisted-by: LLM
Signed-off-by: Dudu Lu <phx0fer@gmail.com>
---
Tested base
-----------
Detected on Linux 7.3-rc4 (commit 93f51579e7df), arm64 QEMU guest.
Re-checked against mainline f47d223627f8 (v7.3-341): the affected code
is unchanged between the two, so the bug is still present there.
Checked against linux-next next-20261021 (commit 9a482184d910): the
affected code is unchanged there as well, so the bug is still present in
linux-next too.

Reproducer status
-----------------
A reproducer was written and executed on an arm64 QEMU guest with
CONFIG_NET_SCH_GRED=m; KCSAN reported the race at the claimed sink.  The
reproducer source is not posted here per Documentation/process/security-bugs.rst;
it is available on request.

Triggering conditions
---------------------
CAP_NET_ADMIN to reconfigure the qdisc; the race is stochastic (long clean
stretches between reports).

Notes
-----
Built and checkpatch-clean: this patch was applied on top of 7.3-rc4
(commit 93f51579e7df) and a full arm64 kernel (defconfig) was built with
it -- no new warnings or errors.  scripts/checkpatch.pl reports 0 errors,
0 warnings.

 net/sched/sch_gred.c | 18 ++++++++++--------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index f04f425c6c44..f3d9a7252949 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -442,13 +442,6 @@ static int gred_change_table_def(struct Qdisc *sch, struct nlattr *dps,
 	red_flags_changed = table->red_flags != sopt->flags;
 	table->red_flags = sopt->flags;
 
-	/*
-	 * Every entry point to GRED is synchronized with the above code
-	 * and the DP is checked against DPs, i.e. shadowed VQs can no
-	 * longer be found so we can unlock right here.
-	 */
-	sch_tree_unlock(sch);
-
 	if (sopt->grio) {
 		gred_enable_rio_mode(table);
 		gred_disable_wred_mode(table);
@@ -474,6 +467,15 @@ static int gred_change_table_def(struct Qdisc *sch, struct nlattr *dps,
 		}
 	}
 
+	/*
+	 * Keep the lock until every field the datapath reads (the
+	 * RIO/WRED mode bits, the per-VQ red_flags and the tab[] slots)
+	 * has been updated: gred_enqueue()/gred_dequeue() read them under
+	 * the same lock, so publishing them after the unlock races with
+	 * the enqueue path.
+	 */
+	sch_tree_unlock(sch);
+
 	gred_offload(sch, TC_GRED_REPLACE);
 	return 0;
 }

Re: [PATCH net] net/sched: gred: keep the qdisc lock while updating the table

[Simon Horman]

On Wed, Oct 21, 2026 at 11:54:33PM +0800, Dudu Lu wrote:
> gred_change_table_def() publishes table->DPs/def/red_flags under
> sch_tree_lock(), drops the lock, and then keeps writing state the
> datapath reads without it:
> 
>   - gred_enable_rio_mode()/gred_disable_rio_mode()/gred_disable_wred_mode()
>     set/clear table->flags;
>   - the red_flags_changed loop writes table->tab[i]->red_flags;
>   - the shadowed-VQ loop frees table->tab[i] and clears the slot.
> 
> gred_enqueue() and gred_dequeue() read those same fields (test_bit on
> table->flags, q->red_flags, t->tab[]/t->DPs) under the same root lock on
> another CPU, so the post-unlock writes race with the enqueue path:
> 
>   BUG: KCSAN: data-race in gred_change_table_def [sch_gred] / gred_enqueue [sch_gred]
>   read (marked) to 0xffff000007ad3b00 of 8 bytes by task 110 on cpu 0:
>    gred_enqueue+0x88/0xc60 [sch_gred]
>    dev_qdisc_enqueue+0x4c/0x19c
>    __dev_queue_xmit+0x10a4/0x1ea4
> 
> The comment justifying the early unlock only covers the DP-vs-DPs check,
> which the shadowed-VQ free loop relies on; it does not cover the scalar
> mode bits and per-VQ red_flags written afterwards.  Move the unlock below
> those updates so everything the datapath reads is published under the
> lock that protects it.
> 
> Fixes: 6639607ed9de ("[PKT_SCHED]: GRED: Use a central table definition change procedure")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
> ---
> Tested base
> -----------
> Detected on Linux 7.3-rc4 (commit 93f51579e7df), arm64 QEMU guest.
> Re-checked against mainline f47d223627f8 (v7.3-341): the affected code
> is unchanged between the two, so the bug is still present there.
> Checked against linux-next next-20261021 (commit 9a482184d910): the
> affected code is unchanged there as well, so the bug is still present in
> linux-next too.
> 
> Reproducer status
> -----------------
> A reproducer was written and executed on an arm64 QEMU guest with
> CONFIG_NET_SCH_GRED=m; KCSAN reported the race at the claimed sink.  The
> reproducer source is not posted here per Documentation/process/security-bugs.rst;
> it is available on request.
> 
> Triggering conditions
> ---------------------
> CAP_NET_ADMIN to reconfigure the qdisc; the race is stochastic (long clean
> stretches between reports).
> 
> Notes
> -----
> Built and checkpatch-clean: this patch was applied on top of 7.3-rc4
> (commit 93f51579e7df) and a full arm64 kernel (defconfig) was built with
> it -- no new warnings or errors.  scripts/checkpatch.pl reports 0 errors,
> 0 warnings.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net/sched: gred: keep the qdisc lock while updating the table

[Daniel Machon]

> gred_change_table_def() publishes table->DPs/def/red_flags under
> sch_tree_lock(), drops the lock, and then keeps writing state the
> datapath reads without it:
> 
>   - gred_enable_rio_mode()/gred_disable_rio_mode()/gred_disable_wred_mode()
>     set/clear table->flags;
>   - the red_flags_changed loop writes table->tab[i]->red_flags;
>   - the shadowed-VQ loop frees table->tab[i] and clears the slot.
> 
> gred_enqueue() and gred_dequeue() read those same fields (test_bit on
> table->flags, q->red_flags, t->tab[]/t->DPs) under the same root lock on
> another CPU, so the post-unlock writes race with the enqueue path:
> 
>   BUG: KCSAN: data-race in gred_change_table_def [sch_gred] / gred_enqueue [sch_gred]
>   read (marked) to 0xffff000007ad3b00 of 8 bytes by task 110 on cpu 0:
>    gred_enqueue+0x88/0xc60 [sch_gred]
>    dev_qdisc_enqueue+0x4c/0x19c
>    __dev_queue_xmit+0x10a4/0x1ea4
> 
> The comment justifying the early unlock only covers the DP-vs-DPs check,
> which the shadowed-VQ free loop relies on; it does not cover the scalar
> mode bits and per-VQ red_flags written afterwards.  Move the unlock below
> those updates so everything the datapath reads is published under the
> lock that protects it.
> 
> Fixes: 6639607ed9de ("[PKT_SCHED]: GRED: Use a central table definition change procedure")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>

Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net] net/sched: gred: keep the qdisc lock while updating the table

[patchwork-bot+netdevbpf]

Hello:

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

On Wed, 21 Oct 2026 23:54:33 +0800 you wrote:
> gred_change_table_def() publishes table->DPs/def/red_flags under
> sch_tree_lock(), drops the lock, and then keeps writing state the
> datapath reads without it:
> 
>   - gred_enable_rio_mode()/gred_disable_rio_mode()/gred_disable_wred_mode()
>     set/clear table->flags;
>   - the red_flags_changed loop writes table->tab[i]->red_flags;
>   - the shadowed-VQ loop frees table->tab[i] and clears the slot.
> 
> [...]

Here is the summary with links:
  - [net] net/sched: gred: keep the qdisc lock while updating the table
    https://git.kernel.org/netdev/net/c/306f788134a2

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