Messages in this thread | | | From | Marco Elver <> | Date | Thu, 12 May 2022 23:18:15 +0200 | Subject | Re: [PATCH net] tcp: Add READ_ONCE() to read tcp_orphan_count |
| |
On Thu, 12 May 2022 at 22:06, Eric Dumazet <edumazet@google.com> wrote: > > On Thu, May 12, 2022 at 3:32 AM Liu Jian <liujian56@huawei.com> wrote: > > > > The tcp_orphan_count per-CPU variable is read locklessly, so this commit > > add the READ_ONCE() to a load in order to avoid below KCSAN warnning: > > > > BUG: KCSAN: data-race in tcp_orphan_count_sum net/ipv4/tcp.c:2476 [inline] > > BUG: KCSAN: data-race in tcp_orphan_update+0x64/0x100 net/ipv4/tcp.c:2487 > > > > race at unknown origin, with read to 0xffff9c63bbdac7a8 of 4 bytes by interrupt on cpu 2: > > tcp_orphan_count_sum net/ipv4/tcp.c:2476 [inline] > > tcp_orphan_update+0x64/0x100 net/ipv4/tcp.c:2487 > > call_timer_fn+0x33/0x210 kernel/time/timer.c:1414 > > > > Fixes: 19757cebf0c5 ("tcp: switch orphan_count to bare per-cpu counters") > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > --- > > net/ipv4/tcp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index cf18fbcbf123..7245609f41e6 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -2718,7 +2718,7 @@ int tcp_orphan_count_sum(void) > > int i, total = 0; > > > > for_each_possible_cpu(i) > > - total += per_cpu(tcp_orphan_count, i); > > + total += READ_ONCE(per_cpu(tcp_orphan_count, i)); > > We might raise the discussion to lkml and/or KCSAN supporters. > > Presumably, all per_cpu() uses in the kernel will have the same issue ? > > By definition per-cpu data can be changed by other cpus. > > So maybe per_cpu() should contain the annotation, instead of having to > annotate all users.
I guess the question is, is it the norm that per_cpu() retrieves data that can legally be modified concurrently, or not. If not, and in most cases it's a bug, the annotations should be here.
Paul, was there any guidance/documentation on this, but I fail to find it right now? (access-marking.txt doesn't say much about per-CPU data.)
Thanks, -- Marco
| |