Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <00000000000089871905cf2b7d09@google.com> <f37aa186-8820-451f-6fa2-eee45799a428@kernel.dk>
 <CANpmjNO1kTswzGp03o_=wMiFekXoq-kvDCy+zKSP3r5+EeOvMg@mail.gmail.com> <b5dd012d-531e-a2ae-18b0-dc2300246298@kernel.dk>
In-Reply-To: <b5dd012d-531e-a2ae-18b0-dc2300246298@kernel.dk>
From:   Marco Elver <elver@google.com>
Date:   Mon, 25 Oct 2021 18:03:35 +0200
Message-ID: <CANpmjNNVh65W00BBRFWwUDp0F4+8RXnU7azqxbpiLuvuev6xEQ@mail.gmail.com>
Subject: Re: [syzbot] KCSAN: data-race in sbitmap_queue_clear /
 sbitmap_queue_clear (3)
To:     Jens Axboe <axboe@kernel.dk>
Cc:     syzbot <syzbot+4f8bfd804b4a1f95b8f6@syzkaller.appspotmail.com>,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzkaller-bugs@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 25 Oct 2021 at 17:40, Jens Axboe <axboe@kernel.dk> wrote:
> On 10/25/21 8:29 AM, Marco Elver wrote:
> > On Mon, 25 Oct 2021 at 15:36, Jens Axboe <axboe@kernel.dk> wrote:
> > [...]
> >>> write to 0xffffe8ffffd145b8 of 4 bytes by interrupt on cpu 1:
> >>>  sbitmap_queue_clear+0xca/0xf0 lib/sbitmap.c:606
> >>>  blk_mq_put_tag+0x82/0x90
> >>>  __blk_mq_free_request+0x114/0x180 block/blk-mq.c:507
> >>>  blk_mq_free_request+0x2c8/0x340 block/blk-mq.c:541
> >>>  __blk_mq_end_request+0x214/0x230 block/blk-mq.c:565
> >>>  blk_mq_end_request+0x37/0x50 block/blk-mq.c:574
> >>>  lo_complete_rq+0xca/0x170 drivers/block/loop.c:541
> >>>  blk_complete_reqs block/blk-mq.c:584 [inline]
> >>>  blk_done_softirq+0x69/0x90 block/blk-mq.c:589
> >>>  __do_softirq+0x12c/0x26e kernel/softirq.c:558
> >>>  run_ksoftirqd+0x13/0x20 kernel/softirq.c:920
> >>>  smpboot_thread_fn+0x22f/0x330 kernel/smpboot.c:164
> >>>  kthread+0x262/0x280 kernel/kthread.c:319
> >>>  ret_from_fork+0x1f/0x30
> >>>
> >>> write to 0xffffe8ffffd145b8 of 4 bytes by interrupt on cpu 0:
> >>>  sbitmap_queue_clear+0xca/0xf0 lib/sbitmap.c:606
> >>>  blk_mq_put_tag+0x82/0x90
> >>>  __blk_mq_free_request+0x114/0x180 block/blk-mq.c:507
> >>>  blk_mq_free_request+0x2c8/0x340 block/blk-mq.c:541
> >>>  __blk_mq_end_request+0x214/0x230 block/blk-mq.c:565
> >>>  blk_mq_end_request+0x37/0x50 block/blk-mq.c:574
> >>>  lo_complete_rq+0xca/0x170 drivers/block/loop.c:541
> >>>  blk_complete_reqs block/blk-mq.c:584 [inline]
> >>>  blk_done_softirq+0x69/0x90 block/blk-mq.c:589
> >>>  __do_softirq+0x12c/0x26e kernel/softirq.c:558
> >>>  run_ksoftirqd+0x13/0x20 kernel/softirq.c:920
> >>>  smpboot_thread_fn+0x22f/0x330 kernel/smpboot.c:164
> >>>  kthread+0x262/0x280 kernel/kthread.c:319
> >>>  ret_from_fork+0x1f/0x30
> >>
> >> This is just a per-cpu alloc hint, it's racy by nature. What's the
> >> preferred way to silence these?
> >
> > That was my guess, but couldn't quite say. We started looking at
> > write/write races as more likely to be harmful (vs. just read/write),
> > and are inclined to let syzbot send out more of such reports. Marking
> > intentional ones would be ideal so we'll be left with the
> > unintentional ones.
> >
> > I would probably use WRITE_ONCE(), just to make sure the compiler
> > doesn't play games here; or if the code is entirely tolerant to even
> > the compiler miscompiling things, wrap the thing in data_race().
>
> It's entirely tolerant, so something like this would do it?

Yup, looks reasonable,

Acked-by: Marco Elver <elver@google.com>

> diff --git a/lib/sbitmap.c b/lib/sbitmap.c
> index c6e2f1f2c4d2..2709ab825499 100644
> --- a/lib/sbitmap.c
> +++ b/lib/sbitmap.c
> @@ -631,7 +631,7 @@ EXPORT_SYMBOL_GPL(sbitmap_queue_wake_up);
>  static inline void sbitmap_update_cpu_hint(struct sbitmap *sb, int cpu, int tag)
>  {
>         if (likely(!sb->round_robin && tag < sb->depth))
> -               *per_cpu_ptr(sb->alloc_hint, cpu) = tag;
> +               data_race(*per_cpu_ptr(sb->alloc_hint, cpu) = tag);
>  }
>
>  void sbitmap_queue_clear_batch(struct sbitmap_queue *sbq, int offset,
>
> --
> Jens Axboe
>