Re: [PATCH] blktrace: Protect q->blk_trace with RCU

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Mar 02, 2020 at 09:19:33PM +0000, Chaitanya Kulkarni wrote:
> By any chance will the following patch be able to get rid of
> the warning ?
> 
> index 4560878f0bac..889555910555 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -1895,9 +1895,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct 
> device *dev,
>                  goto out_unlock_bdev;
>          }
> 
> -       ret = 0;
> -       if (bt == NULL)
> -               ret = blk_trace_setup_queue(q, bdev);
> +       ret = bt == NULL ? blk_trace_setup_queue(q, bdev) : 0;
> 
>          if (ret == 0) {
>                  if (attr == &dev_attr_act_mask)

I don't have a way to run Coverity (I just get periodic reports), but
I don't see the point of this patch.  I assume "bt" can still be NULL
(otherwise there'd be no point in testing it) and we still reference
"bt->pid" below.

> On 03/02/2020 12:41 PM, Bjorn Helgaas wrote:
> > On Thu, Feb 06, 2020 at 03:28:12PM +0100, Jan Kara wrote:
> >
> >> @@ -1844,18 +1896,18 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
> >>   	}
> >>
> >>   	ret = 0;
> >> -	if (q->blk_trace == NULL)
> >> +	if (bt == NULL)
> >>   		ret = blk_trace_setup_queue(q, bdev);
> >>
> >>   	if (ret == 0) {
> >>   		if (attr == &dev_attr_act_mask)
> >> -			q->blk_trace->act_mask = value;
> >> +			bt->act_mask = value;
> >
> > You've likely seen this already, but Coverity complains that "bt" may
> > be a NULL pointer here, since we checked it for NULL above.
> >
> >    CID 1460458:  Null pointer dereferences  (FORWARD_NULL)
> >
> >>   		else if (attr == &dev_attr_pid)
> >> -			q->blk_trace->pid = value;
> >> +			bt->pid = value;
> >>   		else if (attr == &dev_attr_start_lba)
> >> -			q->blk_trace->start_lba = value;
> >> +			bt->start_lba = value;
> >>   		else if (attr == &dev_attr_end_lba)
> >> -			q->blk_trace->end_lba = value;
> >> +			bt->end_lba = value;
> >>   	}
> >>
> >>   out_unlock_bdev:
> >>
> >
>