Re: null_handle_cmd() doesn't initialize data when reading

From: Alexander Potapenko <glider@google.com>
To: Jens Axboe <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org,
	Willem de Bruijn <willemb@google.com>,
	Dmitriy Vyukov <dvyukov@google.com>
Subject: Re: null_handle_cmd() doesn't initialize data when reading
Date: Fri, 22 Nov 2019 12:58:11 +0100	[thread overview]
Message-ID: <CAG_fn=XXsjDsA0_MoTF3XfjSuLCc+6wRaOCY_ZDt661P_rSmOg@mail.gmail.com> (raw)
In-Reply-To: <d204a9d7-3722-5e55-d6cc-3018e366981e@kernel.dk>

On Thu, Nov 21, 2019 at 3:00 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 11/15/19 3:16 AM, Alexander Potapenko wrote:
> > Hi Jens,
> >
> > I'm debugging an issue in nullb driver reported by KMSAN at QEMU startup.
> > There are numerous reports like the one below when checking nullb for
> > different partition types.
> > Basically, read_dev_sector() allocates a cache page which is then
> > wrapped into a bio and passed to the device driver, but never
> > initialized.
> >
> > I've tracked the problem down to a call to null_handle_cmd(cmd,
> > /*sector*/0, /*nr_sectors*/8, /*op*/0).
> > Turns out all the if-branches in this function are skipped, so neither
> > of null_handle_throttled(), null_handle_flush(),
> > null_handle_badblocks(), null_handle_memory_backed(),
> > null_handle_zoned() is executed, and we proceed directly to
> > nullb_complete_cmd().
> >
> > As a result, the pages read from the nullb device are never
> > initialized, at least at boot time.
> > How can we fix this?
> >
> > This bug may also have something to do with
> > https://groups.google.com/d/topic/syzkaller-bugs/d0fmiL9Vi9k/discussion.
>
> Probably just want to have the read path actually memset() them to
> zero, or something like that.

I'm using the following patch in KMSAN tree to suppress these bugs:

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 0e7da5015ccd5..9e8e99bdc0db3 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1235,8 +1235,16 @@ static blk_status_t null_handle_cmd(struct
nullb_cmd *cmd, sector_t sector,
        if (dev->memory_backed)
                cmd->error = null_handle_memory_backed(cmd, op);

-       if (!cmd->error && dev->zoned)
+       if (!cmd->error && dev->zoned) {
                cmd->error = null_handle_zoned(cmd, op, sector, nr_sectors);
+       } else if (dev->queue_mode != NULL_Q_BIO) {
+               /*
+                * TODO(glider): this is a hack to suppress bugs in nullb
+                * driver. I have no idea what I'm doing, better wait for a
+                * proper fix from Jens Axboe.
+                */
+               cmd->error = null_handle_rq(cmd);
+       }

It appears to fix the problem, but I'm not really sure this is the
right thing to do (e.g. whether it covers all invocations of
null_handle_cmd(), probably not)
I don't see an easy way to memset() the pages being read, as there're
no pages at this point.

> --
> Jens Axboe
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
