| Message ID | 1516633309-20803-1-git-send-email-mark.kanda@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-blk: check for NULL BlockDriverState | expand |
On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: > Add a BlockDriverState NULL check to virtio_blk_handle_request() > to prevent a segfault if the drive is forcibly removed using HMP > 'drive_del' (without performing a hotplug 'device_del' first). > > Signed-off-by: Mark Kanda <mark.kanda@oracle.com> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > Reviewed-by: Ameya More <ameya.more@oracle.com> > --- > hw/block/virtio-blk.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index b1532e4..76ddbbf 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > return -1; > } > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block > + * driver state may be NULL and there is nothing left to do. */ > + if (!blk_bs(req->dev->blk)) { Adding Markus Armbruster to check my understanding of drive_del: 1. If id is a node name (e.g. created via blockdev-add) then attempting to remove the root node produces the "Node %s is in use" error. In that case this patch isn't needed. 2. If id is a BlockBackend (e.g. created via -drive) then removing the root node is allowed. The BlockBackend stays in place but blk->root becomes NULL, hence this patch is needed. Markus: What are the valid use cases for #2? If blk->bs becomes NULL I would think a lot more code beyond virtio-blk can segfault. > + virtio_error(vdev, "virtio-blk BlockDriverState is NULL"); > + return -1; > + } > + > /* We always touch the last byte, so just see how big in_iov is. */ > req->in_len = iov_size(in_iov, in_num); > req->in = (void *)in_iov[in_num - 1].iov_base > -- > 1.8.3.1 >
Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: > On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: > > Add a BlockDriverState NULL check to virtio_blk_handle_request() > > to prevent a segfault if the drive is forcibly removed using HMP > > 'drive_del' (without performing a hotplug 'device_del' first). > > > > Signed-off-by: Mark Kanda <mark.kanda@oracle.com> > > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > > Reviewed-by: Ameya More <ameya.more@oracle.com> > > --- > > hw/block/virtio-blk.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index b1532e4..76ddbbf 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > return -1; > > } > > > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block > > + * driver state may be NULL and there is nothing left to do. */ > > + if (!blk_bs(req->dev->blk)) { > > Adding Markus Armbruster to check my understanding of drive_del: > > 1. If id is a node name (e.g. created via blockdev-add) then attempting > to remove the root node produces the "Node %s is in use" error. In > that case this patch isn't needed. > > 2. If id is a BlockBackend (e.g. created via -drive) then removing the > root node is allowed. The BlockBackend stays in place but blk->root > becomes NULL, hence this patch is needed. > > Markus: What are the valid use cases for #2? If blk->bs becomes NULL I > would think a lot more code beyond virtio-blk can segfault. blk->root = NULL is completely normal, it is what happens with removable media when the drive is empty. The problem, which was first reported during the 2.10 RC phase and was worked around in IDE code then, is that Paolo's commit 99723548561 added unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any segfaults that Mark is seeing have the same cause. We do need an in-flight counter even for those requests so that blk_drain() works correctly, so just making the calls condition wouldn't be right. However, this needs to become a separate counter in BlockBackend, and the drain functions must be changed to make use of it. I did post rough patches back then, but they weren't quite ready, and since then they have fallen through the cracks. Kevin
On 1/29/2018 9:41 AM, Kevin Wolf wrote: > Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: >> On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: >>> Add a BlockDriverState NULL check to virtio_blk_handle_request() >>> to prevent a segfault if the drive is forcibly removed using HMP >>> 'drive_del' (without performing a hotplug 'device_del' first). >>> >>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com> >>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> >>> Reviewed-by: Ameya More <ameya.more@oracle.com> >>> --- >>> hw/block/virtio-blk.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >>> index b1532e4..76ddbbf 100644 >>> --- a/hw/block/virtio-blk.c >>> +++ b/hw/block/virtio-blk.c >>> @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) >>> return -1; >>> } >>> >>> + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block >>> + * driver state may be NULL and there is nothing left to do. */ >>> + if (!blk_bs(req->dev->blk)) { >> >> Adding Markus Armbruster to check my understanding of drive_del: >> >> 1. If id is a node name (e.g. created via blockdev-add) then attempting >> to remove the root node produces the "Node %s is in use" error. In >> that case this patch isn't needed. >> >> 2. If id is a BlockBackend (e.g. created via -drive) then removing the >> root node is allowed. The BlockBackend stays in place but blk->root >> becomes NULL, hence this patch is needed. >> >> Markus: What are the valid use cases for #2? If blk->bs becomes NULL I >> would think a lot more code beyond virtio-blk can segfault. > > blk->root = NULL is completely normal, it is what happens with removable > media when the drive is empty. > > The problem, which was first reported during the 2.10 RC phase and was > worked around in IDE code then, is that Paolo's commit 99723548561 added > unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any > segfaults that Mark is seeing have the same cause. That's correct. The segfault I encountered was the bdrv_inc_in_flight() call in blk_aio_prwv(). Thanks, -Mark > We do need an in-flight counter even for those requests so that > blk_drain() works correctly, so just making the calls condition wouldn't > be right. However, this needs to become a separate counter in > BlockBackend, and the drain functions must be changed to make use of it. > > I did post rough patches back then, but they weren't quite ready, and > since then they have fallen through the cracks. > > Kevin >
On Mon, Jan 29, 2018 at 04:41:07PM +0100, Kevin Wolf wrote: > Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: > > On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: > > > Add a BlockDriverState NULL check to virtio_blk_handle_request() > > > to prevent a segfault if the drive is forcibly removed using HMP > > > 'drive_del' (without performing a hotplug 'device_del' first). > > > > > > Signed-off-by: Mark Kanda <mark.kanda@oracle.com> > > > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > > > Reviewed-by: Ameya More <ameya.more@oracle.com> > > > --- > > > hw/block/virtio-blk.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > index b1532e4..76ddbbf 100644 > > > --- a/hw/block/virtio-blk.c > > > +++ b/hw/block/virtio-blk.c > > > @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > > return -1; > > > } > > > > > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block > > > + * driver state may be NULL and there is nothing left to do. */ > > > + if (!blk_bs(req->dev->blk)) { > > > > Adding Markus Armbruster to check my understanding of drive_del: > > > > 1. If id is a node name (e.g. created via blockdev-add) then attempting > > to remove the root node produces the "Node %s is in use" error. In > > that case this patch isn't needed. > > > > 2. If id is a BlockBackend (e.g. created via -drive) then removing the > > root node is allowed. The BlockBackend stays in place but blk->root > > becomes NULL, hence this patch is needed. > > > > Markus: What are the valid use cases for #2? If blk->bs becomes NULL I > > would think a lot more code beyond virtio-blk can segfault. > > blk->root = NULL is completely normal, it is what happens with removable > media when the drive is empty. > > The problem, which was first reported during the 2.10 RC phase and was > worked around in IDE code then, is that Paolo's commit 99723548561 added > unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any > segfaults that Mark is seeing have the same cause. > > We do need an in-flight counter even for those requests so that > blk_drain() works correctly, so just making the calls condition wouldn't > be right. However, this needs to become a separate counter in > BlockBackend, and the drain functions must be changed to make use of it. > > I did post rough patches back then, but they weren't quite ready, and > since then they have fallen through the cracks. Will you send a new version of that patch series? Stefan
Am 30.01.2018 um 13:38 hat Stefan Hajnoczi geschrieben: > On Mon, Jan 29, 2018 at 04:41:07PM +0100, Kevin Wolf wrote: > > Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: > > > On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: > > > > Add a BlockDriverState NULL check to virtio_blk_handle_request() > > > > to prevent a segfault if the drive is forcibly removed using HMP > > > > 'drive_del' (without performing a hotplug 'device_del' first). > > > > > > > > Signed-off-by: Mark Kanda <mark.kanda@oracle.com> > > > > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > > > > Reviewed-by: Ameya More <ameya.more@oracle.com> > > > > --- > > > > hw/block/virtio-blk.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > index b1532e4..76ddbbf 100644 > > > > --- a/hw/block/virtio-blk.c > > > > +++ b/hw/block/virtio-blk.c > > > > @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > > > return -1; > > > > } > > > > > > > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block > > > > + * driver state may be NULL and there is nothing left to do. */ > > > > + if (!blk_bs(req->dev->blk)) { > > > > > > Adding Markus Armbruster to check my understanding of drive_del: > > > > > > 1. If id is a node name (e.g. created via blockdev-add) then attempting > > > to remove the root node produces the "Node %s is in use" error. In > > > that case this patch isn't needed. > > > > > > 2. If id is a BlockBackend (e.g. created via -drive) then removing the > > > root node is allowed. The BlockBackend stays in place but blk->root > > > becomes NULL, hence this patch is needed. > > > > > > Markus: What are the valid use cases for #2? If blk->bs becomes NULL I > > > would think a lot more code beyond virtio-blk can segfault. > > > > blk->root = NULL is completely normal, it is what happens with removable > > media when the drive is empty. > > > > The problem, which was first reported during the 2.10 RC phase and was > > worked around in IDE code then, is that Paolo's commit 99723548561 added > > unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any > > segfaults that Mark is seeing have the same cause. > > > > We do need an in-flight counter even for those requests so that > > blk_drain() works correctly, so just making the calls condition wouldn't > > be right. However, this needs to become a separate counter in > > BlockBackend, and the drain functions must be changed to make use of it. > > > > I did post rough patches back then, but they weren't quite ready, and > > since then they have fallen through the cracks. > > Will you send a new version of that patch series? I would like to continue my work on the drain functions (which this would be a part of) sooner or later, but the work to enable libvirt to use blockdev-add is at a higher priority at the moment. So if you can wait, I'll get to it eventually. If not, feel free to pick up the patches. Kevin
On 01/30/2018 10:56 AM, Kevin Wolf wrote: > Am 30.01.2018 um 13:38 hat Stefan Hajnoczi geschrieben: >> On Mon, Jan 29, 2018 at 04:41:07PM +0100, Kevin Wolf wrote: >>> Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: >>>> On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: >>>>> Add a BlockDriverState NULL check to virtio_blk_handle_request() >>>>> to prevent a segfault if the drive is forcibly removed using HMP >>>>> 'drive_del' (without performing a hotplug 'device_del' first). >>>>> >>>>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com> >>>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> >>>>> Reviewed-by: Ameya More <ameya.more@oracle.com> >>>>> --- >>>>> hw/block/virtio-blk.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>>> >>>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >>>>> index b1532e4..76ddbbf 100644 >>>>> --- a/hw/block/virtio-blk.c >>>>> +++ b/hw/block/virtio-blk.c >>>>> @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) >>>>> return -1; >>>>> } >>>>> >>>>> + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block >>>>> + * driver state may be NULL and there is nothing left to do. */ >>>>> + if (!blk_bs(req->dev->blk)) { >>>> >>>> Adding Markus Armbruster to check my understanding of drive_del: >>>> >>>> 1. If id is a node name (e.g. created via blockdev-add) then attempting >>>> to remove the root node produces the "Node %s is in use" error. In >>>> that case this patch isn't needed. >>>> >>>> 2. If id is a BlockBackend (e.g. created via -drive) then removing the >>>> root node is allowed. The BlockBackend stays in place but blk->root >>>> becomes NULL, hence this patch is needed. >>>> >>>> Markus: What are the valid use cases for #2? If blk->bs becomes NULL I >>>> would think a lot more code beyond virtio-blk can segfault. >>> >>> blk->root = NULL is completely normal, it is what happens with removable >>> media when the drive is empty. >>> >>> The problem, which was first reported during the 2.10 RC phase and was >>> worked around in IDE code then, is that Paolo's commit 99723548561 added >>> unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any >>> segfaults that Mark is seeing have the same cause. >>> >>> We do need an in-flight counter even for those requests so that >>> blk_drain() works correctly, so just making the calls condition wouldn't >>> be right. However, this needs to become a separate counter in >>> BlockBackend, and the drain functions must be changed to make use of it. >>> >>> I did post rough patches back then, but they weren't quite ready, and >>> since then they have fallen through the cracks. >> >> Will you send a new version of that patch series? > > I would like to continue my work on the drain functions (which this > would be a part of) sooner or later, but the work to enable libvirt to > use blockdev-add is at a higher priority at the moment. > > So if you can wait, I'll get to it eventually. If not, feel free to pick > up the patches. > > Kevin > It'd probably be nice for 2.12. I'm not sure I understand the throttling well enough to do it /quickly/ and I have other priorities right now, but let's try to keep this one in mind as something to fix before another release goes by. --js
On Mon, Jan 29, 2018 at 10:13:02AM -0600, Mark Kanda wrote: > > > On 1/29/2018 9:41 AM, Kevin Wolf wrote: > > Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: > > > On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: > > > > Add a BlockDriverState NULL check to virtio_blk_handle_request() > > > > to prevent a segfault if the drive is forcibly removed using HMP > > > > 'drive_del' (without performing a hotplug 'device_del' first). > > > > > > > > Signed-off-by: Mark Kanda <mark.kanda@oracle.com> > > > > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > > > > Reviewed-by: Ameya More <ameya.more@oracle.com> > > > > --- > > > > hw/block/virtio-blk.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > index b1532e4..76ddbbf 100644 > > > > --- a/hw/block/virtio-blk.c > > > > +++ b/hw/block/virtio-blk.c > > > > @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > > > return -1; > > > > } > > > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block > > > > + * driver state may be NULL and there is nothing left to do. */ > > > > + if (!blk_bs(req->dev->blk)) { > > > > > > Adding Markus Armbruster to check my understanding of drive_del: > > > > > > 1. If id is a node name (e.g. created via blockdev-add) then attempting > > > to remove the root node produces the "Node %s is in use" error. In > > > that case this patch isn't needed. > > > > > > 2. If id is a BlockBackend (e.g. created via -drive) then removing the > > > root node is allowed. The BlockBackend stays in place but blk->root > > > becomes NULL, hence this patch is needed. > > > > > > Markus: What are the valid use cases for #2? If blk->bs becomes NULL I > > > would think a lot more code beyond virtio-blk can segfault. > > > > blk->root = NULL is completely normal, it is what happens with removable > > media when the drive is empty. > > > > The problem, which was first reported during the 2.10 RC phase and was > > worked around in IDE code then, is that Paolo's commit 99723548561 added > > unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any > > segfaults that Mark is seeing have the same cause. > > That's correct. The segfault I encountered was the bdrv_inc_in_flight() call > in blk_aio_prwv(). I think we should aim to fix the root cause for QEMU 2.12. blk_*() APIs are supposed to return an error when blk->root is NULL but the blk_aio_*() ones currently crash. If that's not possible then this patch can be merged as a workaround for 2.12 close to the release date. Stefan
On Tue, Jan 30, 2018 at 04:56:12PM +0100, Kevin Wolf wrote: > Am 30.01.2018 um 13:38 hat Stefan Hajnoczi geschrieben: > > On Mon, Jan 29, 2018 at 04:41:07PM +0100, Kevin Wolf wrote: > > > Am 24.01.2018 um 12:31 hat Stefan Hajnoczi geschrieben: > > > > On Mon, Jan 22, 2018 at 09:01:49AM -0600, Mark Kanda wrote: > > > > > Add a BlockDriverState NULL check to virtio_blk_handle_request() > > > > > to prevent a segfault if the drive is forcibly removed using HMP > > > > > 'drive_del' (without performing a hotplug 'device_del' first). > > > > > > > > > > Signed-off-by: Mark Kanda <mark.kanda@oracle.com> > > > > > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > > > > > Reviewed-by: Ameya More <ameya.more@oracle.com> > > > > > --- > > > > > hw/block/virtio-blk.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > index b1532e4..76ddbbf 100644 > > > > > --- a/hw/block/virtio-blk.c > > > > > +++ b/hw/block/virtio-blk.c > > > > > @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) > > > > > return -1; > > > > > } > > > > > > > > > > + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block > > > > > + * driver state may be NULL and there is nothing left to do. */ > > > > > + if (!blk_bs(req->dev->blk)) { > > > > > > > > Adding Markus Armbruster to check my understanding of drive_del: > > > > > > > > 1. If id is a node name (e.g. created via blockdev-add) then attempting > > > > to remove the root node produces the "Node %s is in use" error. In > > > > that case this patch isn't needed. > > > > > > > > 2. If id is a BlockBackend (e.g. created via -drive) then removing the > > > > root node is allowed. The BlockBackend stays in place but blk->root > > > > becomes NULL, hence this patch is needed. > > > > > > > > Markus: What are the valid use cases for #2? If blk->bs becomes NULL I > > > > would think a lot more code beyond virtio-blk can segfault. > > > > > > blk->root = NULL is completely normal, it is what happens with removable > > > media when the drive is empty. > > > > > > The problem, which was first reported during the 2.10 RC phase and was > > > worked around in IDE code then, is that Paolo's commit 99723548561 added > > > unconditional bdrv_inc/dec_in_flight() calls. I am pretty sure that any > > > segfaults that Mark is seeing have the same cause. > > > > > > We do need an in-flight counter even for those requests so that > > > blk_drain() works correctly, so just making the calls condition wouldn't > > > be right. However, this needs to become a separate counter in > > > BlockBackend, and the drain functions must be changed to make use of it. > > > > > > I did post rough patches back then, but they weren't quite ready, and > > > since then they have fallen through the cracks. > > > > Will you send a new version of that patch series? > > I would like to continue my work on the drain functions (which this > would be a part of) sooner or later, but the work to enable libvirt to > use blockdev-add is at a higher priority at the moment. > > So if you can wait, I'll get to it eventually. If not, feel free to pick > up the patches. Okay, I'll let you know early next week whether I am able to work on it. Stefan
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index b1532e4..76ddbbf 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -507,6 +507,13 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb) return -1; } + /* If the drive was forcibly removed (e.g. HMP 'drive_del'), the block + * driver state may be NULL and there is nothing left to do. */ + if (!blk_bs(req->dev->blk)) { + virtio_error(vdev, "virtio-blk BlockDriverState is NULL"); + return -1; + } + /* We always touch the last byte, so just see how big in_iov is. */ req->in_len = iov_size(in_iov, in_num); req->in = (void *)in_iov[in_num - 1].iov_base