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

[Daniel Machon <daniel.machon@microchip.com>]

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