Re: [PATCH] xen-blkfront: Handle NULL gendisk

[Jason Andryuk]

On Thu, Jun 2, 2022 at 2:02 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 01, 2022 at 03:53:41PM -0400, Jason Andryuk wrote:
> > When a VBD is not fully created and then closed, the kernel can have a
> > NULL pointer dereference:
> >

> >
> > info->rq and info->gd are only set in blkfront_connect(), which is
> > called for state 4 (XenbusStateConnected).  Guard against using NULL
> > variables in blkfront_closing() to avoid the issue.
> >
> > The rest of blkfront_closing looks okay.  If info->nr_rings is 0, then
> > for_each_rinfo won't do anything.
> >
> > blkfront_remove also needs to check for non-NULL pointers before
> > cleaning up the gendisk and request queue.
> >
> > Fixes: 05d69d950d9d "xen-blkfront: sanitize the removal state machine"
> > Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
>
> Tis looks ok, but do we have anything that prevents races between
> blkfront_connect, blkfront_closing and blkfront_remove?

Thanks for taking a look, Christoph.

blkfront_connect and blkfront_closing are called by the state machine
in blkback_changed.  blkback_changed is the xenbus_driver
.otherend_changed callback.  The xenwatch kthread calls callbacks
synchronously and one at a time, so that seems okay today.

blkfront_remove is the xenbus_driver .remove callback, so it is tied
to the life cycle of the device.  It's called after the
otherend_changed callback is unregistered, so those won't run when
blkfront_remove is running.

Given that, I think it's okay.

Regards,
Jason