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

[Alexander Potapenko <glider@google.com>]

(+Damien Le Moal)
(more context at
https://lore.kernel.org/linux-block/CAG_fn=VBHmBgqLi35tD27NRLH2tEZLH=Y+rTfZ3rKNz9ipG+jQ@mail.gmail.com/)

On Mon, Nov 25, 2019 at 5:01 AM Chaitanya Kulkarni
<Chaitanya.Kulkarni@wdc.com> wrote:
>
> On 11/22/2019 03:58 AM, Alexander Potapenko wrote:
> > 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);
> > +       }
>
> This is just based on what I read in the code, I don't have
> the kmsan support.
>
> null_handle_rq() should not be called without checking the
> dev->memory_backed value since it handles memory backed operation,
> (it may be working because of the correct error handling done
> in the copy_from_nullb() when t_page == NULL).

Thanks for the explanation!
The code has changed recently, and my patch does not apply anymore,
yet the problem still persists.
I ended up just calling null_handle_rq() at the end of
null_process_cmd(), but we probably need a cleaner fix.

> Possible explanation could be for the above code fixing the problem
> is:-
>
> null_handle_cmd()
>         None of the ifs are handled (since it is default behavior)
>         calls null_handle_rq()
>                 Processes each bvec in this request :-
>                 rq_for_each_segment()
>                         calls null_transfer()
>                                 calls copy_from_null()
>
> Now here in copy_from_null(), null_lookup_page() returns
> NULL since it is not present in the radix tree(dev and cache
> since its !membacked && !cache_size I assume since I don't have
> the command line of modprobe null_blk),
>
> and then does the dest memset():-
>
> (from linux-block for-next)
> 1009                 if (!t_page) {
> 1010                         memset(dst + off + count, 0, temp);
> 1011                         goto next;
> 1012                 }
>
> which initializes the destination page to 0. This is what Jens
> is referring to I guess that we need to memset() it for the
> read when !membacked.
>
> It will be great if you can verify the above code path on your
> setup with above memset().
>
> The above patch does fix the problem but it is very confusing.
>
> What we need is to traverse the rq bvec and actually initialize
> the bvec.page and repeat the pattern present in the copy_from_null()
> in such a way this common code is shared between membacked and
> non-membacked mode for REQ_OP_READ.
>
> Jens can correct me if I'm wrong.
>
> I'll be happy to look into this if Jens is okay.
>
> >
> > 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.
> >
>
>
> >> >--
>


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