* [Qemu-devel] [PATCH] nvme: Set number of queues later in nvme_init()
@ 2019-07-10 14:57 Michal Privoznik
2019-07-10 15:34 ` Philippe Mathieu-Daudé
2019-07-10 21:07 ` [Qemu-devel] " Max Reitz
0 siblings, 2 replies; 4+ messages in thread
From: Michal Privoznik @ 2019-07-10 14:57 UTC (permalink / raw)
To: qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz
When creating the admin queue in nvme_init() the variable that
holds the number of queues created is modified before actual
queue creation. This is a problem because if creating the queue
fails then the variable is left in inconsistent state. This was
actually observed when I tried to hotplug a nvme disk. The
control got to nvme_file_open() which called nvme_init() which
failed and thus nvme_close() was called which in turn called
nvme_free_queue_pair() with queue being NULL. This lead to an
instant crash:
#0 0x000055d9507ec211 in nvme_free_queue_pair (bs=0x55d952ddb880, q=0x0) at block/nvme.c:164
#1 0x000055d9507ee180 in nvme_close (bs=0x55d952ddb880) at block/nvme.c:729
#2 0x000055d9507ee3d5 in nvme_file_open (bs=0x55d952ddb880, options=0x55d952bb1410, flags=147456, errp=0x7ffd8e19e200) at block/nvme.c:781
#3 0x000055d9507629f3 in bdrv_open_driver (bs=0x55d952ddb880, drv=0x55d95109c1e0 <bdrv_nvme>, node_name=0x0, options=0x55d952bb1410, open_flags=147456, errp=0x7ffd8e19e310) at block.c:1291
#4 0x000055d9507633d6 in bdrv_open_common (bs=0x55d952ddb880, file=0x0, options=0x55d952bb1410, errp=0x7ffd8e19e310) at block.c:1551
#5 0x000055d950766881 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d952bb1410, flags=32768, parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, errp=0x7ffd8e19e510) at block.c:3063
#6 0x000055d950765ae4 in bdrv_open_child_bs (filename=0x0, options=0x55d9541cdff0, bdref_key=0x55d950af33aa "file", parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, allow_none=true, errp=0x7ffd8e19e510) at block.c:2712
#7 0x000055d950766633 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d9541cdff0, flags=0, parent=0x0, child_role=0x0, errp=0x7ffd8e19e908) at block.c:3011
#8 0x000055d950766dba in bdrv_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block.c:3156
#9 0x000055d9507cb635 in blk_new_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block/block-backend.c:389
#10 0x000055d950465ec5 in blockdev_init (file=0x0, bs_opts=0x55d953d00390, errp=0x7ffd8e19e908) at blockdev.c:602
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
block/nvme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/nvme.c b/block/nvme.c
index 73ed5fa75f..9896b7f7c6 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -613,12 +613,12 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
/* Set up admin queue. */
s->queues = g_new(NVMeQueuePair *, 1);
- s->nr_queues = 1;
s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
if (!s->queues[0]) {
ret = -EINVAL;
goto out;
}
+ s->nr_queues = 1;
QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
s->regs->asq = cpu_to_le64(s->queues[0]->sq.iova);
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Set number of queues later in nvme_init()
2019-07-10 14:57 [Qemu-devel] [PATCH] nvme: Set number of queues later in nvme_init() Michal Privoznik
@ 2019-07-10 15:34 ` Philippe Mathieu-Daudé
2019-07-10 17:08 ` [Qemu-devel] [Qemu-block] " Maxim Levitsky
2019-07-10 21:07 ` [Qemu-devel] " Max Reitz
1 sibling, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-10 15:34 UTC (permalink / raw)
To: Michal Privoznik, qemu-block; +Cc: fam, kwolf, qemu-devel, mreitz
On 7/10/19 4:57 PM, Michal Privoznik wrote:
> When creating the admin queue in nvme_init() the variable that
> holds the number of queues created is modified before actual
> queue creation. This is a problem because if creating the queue
> fails then the variable is left in inconsistent state. This was
> actually observed when I tried to hotplug a nvme disk. The
> control got to nvme_file_open() which called nvme_init() which
> failed and thus nvme_close() was called which in turn called
> nvme_free_queue_pair() with queue being NULL. This lead to an
> instant crash:
>
> #0 0x000055d9507ec211 in nvme_free_queue_pair (bs=0x55d952ddb880, q=0x0) at block/nvme.c:164
> #1 0x000055d9507ee180 in nvme_close (bs=0x55d952ddb880) at block/nvme.c:729
> #2 0x000055d9507ee3d5 in nvme_file_open (bs=0x55d952ddb880, options=0x55d952bb1410, flags=147456, errp=0x7ffd8e19e200) at block/nvme.c:781
> #3 0x000055d9507629f3 in bdrv_open_driver (bs=0x55d952ddb880, drv=0x55d95109c1e0 <bdrv_nvme>, node_name=0x0, options=0x55d952bb1410, open_flags=147456, errp=0x7ffd8e19e310) at block.c:1291
> #4 0x000055d9507633d6 in bdrv_open_common (bs=0x55d952ddb880, file=0x0, options=0x55d952bb1410, errp=0x7ffd8e19e310) at block.c:1551
> #5 0x000055d950766881 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d952bb1410, flags=32768, parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, errp=0x7ffd8e19e510) at block.c:3063
> #6 0x000055d950765ae4 in bdrv_open_child_bs (filename=0x0, options=0x55d9541cdff0, bdref_key=0x55d950af33aa "file", parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, allow_none=true, errp=0x7ffd8e19e510) at block.c:2712
> #7 0x000055d950766633 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d9541cdff0, flags=0, parent=0x0, child_role=0x0, errp=0x7ffd8e19e908) at block.c:3011
> #8 0x000055d950766dba in bdrv_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block.c:3156
> #9 0x000055d9507cb635 in blk_new_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block/block-backend.c:389
> #10 0x000055d950465ec5 in blockdev_init (file=0x0, bs_opts=0x55d953d00390, errp=0x7ffd8e19e908) at blockdev.c:602
>
Fixes: bdd6a90a9e5
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> block/nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 73ed5fa75f..9896b7f7c6 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -613,12 +613,12 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>
> /* Set up admin queue. */
> s->queues = g_new(NVMeQueuePair *, 1);
> - s->nr_queues = 1;
> s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
> if (!s->queues[0]) {
> ret = -EINVAL;
> goto out;
> }
> + s->nr_queues = 1;
> QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
> s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
> s->regs->asq = cpu_to_le64(s->queues[0]->sq.iova);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] nvme: Set number of queues later in nvme_init()
2019-07-10 15:34 ` Philippe Mathieu-Daudé
@ 2019-07-10 17:08 ` Maxim Levitsky
0 siblings, 0 replies; 4+ messages in thread
From: Maxim Levitsky @ 2019-07-10 17:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michal Privoznik, qemu-block
Cc: fam, kwolf, qemu-devel, mreitz
On Wed, 2019-07-10 at 17:34 +0200, Philippe Mathieu-Daudé wrote:
> On 7/10/19 4:57 PM, Michal Privoznik wrote:
> > When creating the admin queue in nvme_init() the variable that
> > holds the number of queues created is modified before actual
> > queue creation. This is a problem because if creating the queue
> > fails then the variable is left in inconsistent state. This was
> > actually observed when I tried to hotplug a nvme disk. The
> > control got to nvme_file_open() which called nvme_init() which
> > failed and thus nvme_close() was called which in turn called
> > nvme_free_queue_pair() with queue being NULL. This lead to an
> > instant crash:
> >
> > #0 0x000055d9507ec211 in nvme_free_queue_pair (bs=0x55d952ddb880, q=0x0) at block/nvme.c:164
> > #1 0x000055d9507ee180 in nvme_close (bs=0x55d952ddb880) at block/nvme.c:729
> > #2 0x000055d9507ee3d5 in nvme_file_open (bs=0x55d952ddb880, options=0x55d952bb1410, flags=147456, errp=0x7ffd8e19e200) at block/nvme.c:781
> > #3 0x000055d9507629f3 in bdrv_open_driver (bs=0x55d952ddb880, drv=0x55d95109c1e0 <bdrv_nvme>, node_name=0x0, options=0x55d952bb1410, open_flags=147456, errp=0x7ffd8e19e310) at block.c:1291
> > #4 0x000055d9507633d6 in bdrv_open_common (bs=0x55d952ddb880, file=0x0, options=0x55d952bb1410, errp=0x7ffd8e19e310) at block.c:1551
> > #5 0x000055d950766881 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d952bb1410, flags=32768, parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, errp=0x7ffd8e19e510)
> > at block.c:3063
> > #6 0x000055d950765ae4 in bdrv_open_child_bs (filename=0x0, options=0x55d9541cdff0, bdref_key=0x55d950af33aa "file", parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>,
> > allow_none=true, errp=0x7ffd8e19e510) at block.c:2712
> > #7 0x000055d950766633 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d9541cdff0, flags=0, parent=0x0, child_role=0x0, errp=0x7ffd8e19e908) at block.c:3011
> > #8 0x000055d950766dba in bdrv_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block.c:3156
> > #9 0x000055d9507cb635 in blk_new_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block/block-backend.c:389
> > #10 0x000055d950465ec5 in blockdev_init (file=0x0, bs_opts=0x55d953d00390, errp=0x7ffd8e19e908) at blockdev.c:602
> >
>
> Fixes: bdd6a90a9e5
>
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> > ---
> > block/nvme.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/nvme.c b/block/nvme.c
> > index 73ed5fa75f..9896b7f7c6 100644
> > --- a/block/nvme.c
> > +++ b/block/nvme.c
> > @@ -613,12 +613,12 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
> >
> > /* Set up admin queue. */
> > s->queues = g_new(NVMeQueuePair *, 1);
> > - s->nr_queues = 1;
> > s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
> > if (!s->queues[0]) {
> > ret = -EINVAL;
> > goto out;
> > }
> > + s->nr_queues = 1;
> > QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
> > s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
> > s->regs->asq = cpu_to_le64(s->queues[0]->sq.iova);
> >
>
>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] nvme: Set number of queues later in nvme_init()
2019-07-10 14:57 [Qemu-devel] [PATCH] nvme: Set number of queues later in nvme_init() Michal Privoznik
2019-07-10 15:34 ` Philippe Mathieu-Daudé
@ 2019-07-10 21:07 ` Max Reitz
1 sibling, 0 replies; 4+ messages in thread
From: Max Reitz @ 2019-07-10 21:07 UTC (permalink / raw)
To: Michal Privoznik, qemu-block; +Cc: fam, kwolf, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2547 bytes --]
On 10.07.19 16:57, Michal Privoznik wrote:
> When creating the admin queue in nvme_init() the variable that
> holds the number of queues created is modified before actual
> queue creation. This is a problem because if creating the queue
> fails then the variable is left in inconsistent state. This was
> actually observed when I tried to hotplug a nvme disk. The
> control got to nvme_file_open() which called nvme_init() which
> failed and thus nvme_close() was called which in turn called
> nvme_free_queue_pair() with queue being NULL. This lead to an
> instant crash:
>
> #0 0x000055d9507ec211 in nvme_free_queue_pair (bs=0x55d952ddb880, q=0x0) at block/nvme.c:164
> #1 0x000055d9507ee180 in nvme_close (bs=0x55d952ddb880) at block/nvme.c:729
> #2 0x000055d9507ee3d5 in nvme_file_open (bs=0x55d952ddb880, options=0x55d952bb1410, flags=147456, errp=0x7ffd8e19e200) at block/nvme.c:781
> #3 0x000055d9507629f3 in bdrv_open_driver (bs=0x55d952ddb880, drv=0x55d95109c1e0 <bdrv_nvme>, node_name=0x0, options=0x55d952bb1410, open_flags=147456, errp=0x7ffd8e19e310) at block.c:1291
> #4 0x000055d9507633d6 in bdrv_open_common (bs=0x55d952ddb880, file=0x0, options=0x55d952bb1410, errp=0x7ffd8e19e310) at block.c:1551
> #5 0x000055d950766881 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d952bb1410, flags=32768, parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, errp=0x7ffd8e19e510) at block.c:3063
> #6 0x000055d950765ae4 in bdrv_open_child_bs (filename=0x0, options=0x55d9541cdff0, bdref_key=0x55d950af33aa "file", parent=0x55d9538ce420, child_role=0x55d950eaade0 <child_file>, allow_none=true, errp=0x7ffd8e19e510) at block.c:2712
> #7 0x000055d950766633 in bdrv_open_inherit (filename=0x0, reference=0x0, options=0x55d9541cdff0, flags=0, parent=0x0, child_role=0x0, errp=0x7ffd8e19e908) at block.c:3011
> #8 0x000055d950766dba in bdrv_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block.c:3156
> #9 0x000055d9507cb635 in blk_new_open (filename=0x0, reference=0x0, options=0x55d953d00390, flags=0, errp=0x7ffd8e19e908) at block/block-backend.c:389
> #10 0x000055d950465ec5 in blockdev_init (file=0x0, bs_opts=0x55d953d00390, errp=0x7ffd8e19e908) at blockdev.c:602
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> block/nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-10 21:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-10 14:57 [Qemu-devel] [PATCH] nvme: Set number of queues later in nvme_init() Michal Privoznik
2019-07-10 15:34 ` Philippe Mathieu-Daudé
2019-07-10 17:08 ` [Qemu-devel] [Qemu-block] " Maxim Levitsky
2019-07-10 21:07 ` [Qemu-devel] " Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.