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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Cc: Jan Kara <jack@suse.cz>, Jens Axboe <axboe@kernel.dk>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"tristmd@gmail.com" <tristmd@gmail.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH] blktrace: Protect q->blk_trace with RCU
Date: Mon, 2 Mar 2020 15:59:00 -0600	[thread overview]
Message-ID: <20200302215900.GA168003@google.com> (raw)
In-Reply-To: <BYAPR04MB574957E5116240523362B1DC86E70@BYAPR04MB5749.namprd04.prod.outlook.com>

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