Re: [PATCH 1/2] pktcdvd: Fix pkt_setup_dev() error path

From: Ming Lei <ming.lei@redhat.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Bart Van Assche <bvanassche@acm.org>,
	Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
	Tejun Heo <tj@kernel.org>,
	"Maciej S . Szmigiero" <mail@maciej.szmigiero.name>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 1/2] pktcdvd: Fix pkt_setup_dev() error path
Date: Sat, 25 Apr 2020 17:17:00 +0800	[thread overview]
Message-ID: <20200425091601.GA492109@T590> (raw)
In-Reply-To: <CAB=NE6Uhn88Vrymb2x+=7YmieRguGKm9Dk1LiDqw6oggZJpp8g@mail.gmail.com>

On Fri, Apr 24, 2020 at 07:39:47PM -0600, Luis Chamberlain wrote:
> So I hopped on a time machine to revise some old collateral due to
> 523e1d399ce ("block: make gendisk hold a reference to its queue")
> merged on v3.2 which added the conditional check for the disk->queue
> before calling blk_put_queue() on release_disk(). I started wondering
> *why* the conditional was added, but I checked the original patch and
> I could not find discussion around it.
> 
> Tejun, do you call why you added that conditional on
> 
> if (disk->queue)
>   blk_put_queue(disk->queue);
> 
> This patch however struck me as one I should highlight, since I'm
> reviewing all this now and dealing with adding error paths on
> add_disk(). Below some notes.

disk->queue is assigned by drivers, I guess that is why the check
is needed, given the disk may be released in error path before driver
assigns queue to it.

Also some driver may only allocate disk and not add disk, then not
necessary to assign disk->queue, such as drivers/scsi/sg.c

> 
> On Tue, Jan 2, 2018 at 1:40 PM Bart Van Assche <bart.vanassche@wdc.com> wrote:
> >
> > Commit 523e1d399ce0 ("block: make gendisk hold a reference to its queue")
> > modified add_disk() and disk_release() but did not update any of the
> > error paths that trigger a put_disk() call after disk->queue has been
> > assigned. That introduced the following behavior in the pktcdvd driver
> > if pkt_new_dev() fails:
> >
> > Kernel BUG at 00000000e98fd882 [verbose debug info unavailable]
> >
> > Since disk_release() calls blk_put_queue() anyway if disk->queue != NULL,
> > fix this by removing the blk_cleanup_queue() call from the pkt_setup_dev()
> > error path.
> >
> > Fixes: commit 523e1d399ce0 ("block: make gendisk hold a reference to its queue")
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > Cc: <stable@vger.kernel.org> # v3.2
> > ---
> >  drivers/block/pktcdvd.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> > index 67974796c350..2659b2534073 100644
> > --- a/drivers/block/pktcdvd.c
> > +++ b/drivers/block/pktcdvd.c
> > @@ -2745,7 +2745,7 @@ static int pkt_setup_dev(dev_t dev, dev_t* pkt_dev)
> >         pd->pkt_dev = MKDEV(pktdev_major, idx);
> >         ret = pkt_new_dev(pd, dev);
> >         if (ret)
> > -               goto out_new_dev;
> > +               goto out_mem2;
> >
> >         /* inherit events of the host device */
> >         disk->events = pd->bdev->bd_disk->events;
> > @@ -2763,8 +2763,6 @@ static int pkt_setup_dev(dev_t dev, dev_t* pkt_dev)
> >         mutex_unlock(&ctl_mutex);
> >         return 0;
> >
> > -out_new_dev:
> > -       blk_cleanup_queue(disk->queue);
> >  out_mem2:
> >         put_disk(disk);
> >  out_mem:
> > --
> 
> As we have it now drivers *do* call blk_cleanup_queue() on error paths
> prior to add_disk(). An example today is on drivers/block/loop.c where
> in loop_add(), if alloc_disk() fails we call  blk_cleanup_queue()
> *but* this error path *never* called put_disk() as
> drivers/block/pktcdvd.c did on error, and that is because it doesn't
> need to as the last error-path-induced call was alloc_disk(). So it
> doesn't need to free the disk as its not created on the error path of
> loop_add().
> 
> This will of course change once we make add_disk() return int, and
> capture errors, and it brings the question if we want to follow
> similar strategy for other drivers, however note that blk_put_queue()
> doesn't do everything blk_cleanup_queue() does, and in fact
> blk_cleanup_queue() states it sets up "the appropriate flags" *and*
> then calls blk_put_queue().
> 
> We'll have a a bit more collateral evolutions if we embrace the
> strategy in this commit, for those drivers that wish to start taking
> advantage of the error checks, but other then considering this, I
> thought it would be good to think about the fact that *today* we call
> blk_cleanup_queue() on error paths *without* the disk being yet
> associated either. This, in spite of the fact that the way we designed

Some drivers may have only request queue, and not have disk, such as
NVMe's admin queue, so I think blk_cleanup_queue() has to cover this
case.

> the queue, it sits on top of the disk from a kobject perspective once
> registered. Since we call blk_cleanup_queue() on error paths today --
> without a disk parent being possible -- it means nothing on
> blk_cleanup_queue() should not rely on it having a functional disk. Do
> we want to keep it that way? If we keep the practice of drivers using

Yes, see the reason above.

Thanks,
Ming