Re: [PATCH net] ipv6: use READ_ONCE() in ipv6_flowlabel_get()

[David Laight]

On Tue, 2 Jun 2026 01:10:49 -0700
Eric Dumazet <edumazet@xxxxxxxxxx> wrote:

> On Tue, Jun 2, 2026 at 1:00 AM David Laight
> <david.laight.linux@xxxxxxxxx> wrote:
> >
> > On Mon,  1 Jun 2026 23:14:44 +0000
> > Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> >  
> > > From: David Laight <david.laight.linux@xxxxxxxxx>
> > > Date: Mon, 1 Jun 2026 22:31:22 +0100  
> > > > On Mon, 1 Jun 2026 05:36:37 -0700
> > > > Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > > >  
> > > > > On Mon, Jun 1, 2026 at 5:22 AM David Laight
> > > > > <david.laight.linux@xxxxxxxxx> wrote:  
> > > > > >
> > > > > > On Sun, 31 May 2026 23:39:46 +0800
> > > > > > Runyu Xiao <runyu.xiao@xxxxxxxxxx> wrote:
> > > > > >  
> > > > > > > ipv6_flowlabel_get() still reads the shared per-net sysctl fields
> > > > > > > flowlabel_consistency and flowlabel_state_ranges with plain loads,
> > > > > > > while writers update them through proc_dou8vec_minmax(). These checks
> > > > > > > run in the live IPV6_FLOWLABEL_MGR path, so lockless plain reads leave
> > > > > > > KCSAN-visible data races and can make the policy checks observe stale or
> > > > > > > inconsistent values.
> > > > > > >
> > > > > > > The race can be reached on a running system by toggling
> > > > > > > /proc/sys/net/ipv6/flowlabel_consistency and
> > > > > > > /proc/sys/net/ipv6/flowlabel_state_ranges while another task repeatedly
> > > > > > > issues IPV6_FLOWLABEL_MGR requests with IPV6_FL_F_REFLECT or a
> > > > > > > state-ranges flow label.
> > > > > > >
> > > > > > > This issue was first flagged by our static analysis tool while scanning
> > > > > > > lockless IPv6 sysctl readers, then manually audited on Linux v6.18.21.
> > > > > > > The IPV6_FLOWLABEL_MGR paths were runtime-reproduced with QEMU/KCSAN by
> > > > > > > concurrently flipping the two sysctls while TCP reflect and UDP
> > > > > > > state-ranges setsockopt actors exercised ipv6_flowlabel_get(). KCSAN
> > > > > > > reported races between proc_dou8vec_minmax() and the two plain-load
> > > > > > > sites in ipv6_flowlabel_get().
> > > > > > >
> > > > > > > A narrower second-round UDPv6 + IPV6_AUTOFLOWLABEL send-side reproducer
> > > > > > > also hit the inline ip6_make_flowlabel() reader through
> > > > > > > __ip6_make_skb() / proc_dou8vec_minmax(), but that site is already
> > > > > > > fixed in this tree by commit ded139b59b5d
> > > > > > > ("ipv6: annotate data-races from ip6_make_flowlabel()"). The remaining
> > > > > > > plain readers in this tree are both in ipv6_flowlabel_get().
> > > > > > >
> > > > > > > Use READ_ONCE() for those remaining sysctl reads so they follow the same
> > > > > > > lockless reader contract already used by other IPv6 sysctl readers.
> > > > > > >
> > > > > > > Build-tested by compiling net/ipv6/ip6_flowlabel.o on x86_64.
> > > > > > >
> > > > > > > Representative QEMU/KCSAN reports from the two target reader paths:
> > > > > > >
> > > > > > >   BUG: KCSAN: data-race in ipv6_flowlabel_opt / proc_dou8vec_minmax
> > > > > > >   write: proc_dou8vec_minmax+0x206/0x220
> > > > > > >   read:  ipv6_flowlabel_opt+0x6d8/0xd20
> > > > > > >          do_ipv6_setsockopt+0x873/0x2220
> > > > > > >          tcp_setsockopt+0x72/0xb0
> > > > > > >
> > > > > > >   BUG: KCSAN: data-race in ipv6_flowlabel_opt / proc_dou8vec_minmax
> > > > > > >   write: proc_dou8vec_minmax+0x206/0x220
> > > > > > >   read:  ipv6_flowlabel_opt+0x129/0xd20
> > > > > > >          do_ipv6_setsockopt+0x873/0x2220
> > > > > > >          udpv6_setsockopt+0x21/0x40
> > > > > > >
> > > > > > > Fixes: 6444f72b4b74 ("ipv6: add flowlabel_consistency sysctl")
> > > > > > > Fixes: 82a584b7cd36 ("ipv6: Flow label state ranges")
> > > > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > > > Signed-off-by: Runyu Xiao <runyu.xiao@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >  net/ipv6/ip6_flowlabel.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >A
> > > > > > > diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> > > > > > > index b1ccdf0dc646..1ab5ad0dcf24 100644
> > > > > > > --- a/net/ipv6/ip6_flowlabel.c
> > > > > > > +++ b/net/ipv6/ip6_flowlabel.c
> > > > > > > @@ -620,7 +620,7 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq,
> > > > > > >       int err;
> > > > > > >
> > > > > > >       if (freq->flr_flags & IPV6_FL_F_REFLECT) {
> > > > > > > -             if (net->ipv6.sysctl.flowlabel_consistency) {
> > > > > > > +             if (READ_ONCE(net->ipv6.sysctl.flowlabel_consistency)) {  
> > > > > >
> > > > > > That can't actually fix anything.  
> > > > >
> > > > > It fixes a KCSAN splat.
> > > > >
> > > > > If you think you can fix KCSAN instead, please do so.  
> >
> > ipv6.h has:
> >         u8 flowlabel_consistency;
> >
> > KCSAN probably shouldn't care about byte reads.  
> 
> KCSAN detects more than just load/store tearing. Here is a summary:
> 
> Purpose: KCSAN identifies data races, which are a common source of
> correctness, stability,
> and security bugs in concurrent systems like the Linux kernel.

Ok, it can pick up CSE type issues as well.
But this one is still a false positive.

-- David

> 
> Mechanism: It is a compiler-instrumentation-based tool. During
> compilation, special code is added to monitor memory accesses.
> At runtime, KCSAN detects when multiple threads access the same memory
> location without proper synchronization,
> and at least one of those accesses is a write.
> 
> Operation: KCSAN performs its analysis at runtime, reporting data
> races that actually occur or nearly occur during code execution.
> While powerful and scalable across the entire kernel, this
> instrumentation can significantly slow down kernel execution.
> 
> Impact: KCSAN has been instrumental in finding and fixing numerous
> concurrency bugs.
> For example, it has led to the addition of annotations like
> READ_ONCE() and WRITE_ONCE()
> in kernel code (e.g., in the TCP/IPv6 stack) to properly handle
> lockless reads and writes and resolve reported data races.
> 
> 
> 
> 
> >  
> > > >
> > > > It is a false positive.  
> > >
> > > It's not.
> > >
> > >  
> > > > (Which I think you also said in a different email.  
> > >
> > > I guess you meant this one ?
> > > https://lore.kernel.org/netdev/20260601074201.1186061-1-runyu.xiao@xxxxxxxxxx/
> > >
> > > This is different because, in addition to Eric's comment, IPv6
> > > address is 128-bit and data-race is inevitable without locking
> > > unless CPU supports native 128-bit read/write; we already do
> > > load/store-tearing of 128bit with u32/u64.  
> >
> > But the code isn't looking at a 128bit value, it is only doing a check
> > for zero (and READ_ONCE() doesn't support 128bit values).
> > If there is no locking the value can change just before/after the test.
> > Even if it were subject to read/write tearing absolutely the worst that
> > could happen is a zero being detected when the value changes between
> > two non-zero values.
> > That isn't relevant here - it is just a boolean.  
> 
> It is completely relevant. If you disagree, please fix KCSAN.