* [PATCH V2 1/2] virtio_pmem: initialize provider_data through nd_region_desc
@ 2022-06-27 6:29 Jason Wang
2022-06-27 6:29 ` [PATCH V2 2/2] virtio_pmem: set device ready in probe() Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2022-06-27 6:29 UTC (permalink / raw)
To: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny, nvdimm,
linux-kernel, pankaj.gupta, mst
Cc: Jason Wang
We used to initialize the provider_data manually after
nvdimm_pemm_region_create(). This seems to be racy if the flush is
issued before the initialization of provider_data[1]. Fixing this by
initializing the provider_data through nd_region_desc to make sure the
provider_data is ready after the pmem is created.
[1]:
[ 80.152281] nd_pmem namespace0.0: unable to guarantee persistence of writes
[ 92.393956] BUG: kernel NULL pointer dereference, address: 0000000000000318
[ 92.394551] #PF: supervisor read access in kernel mode
[ 92.394955] #PF: error_code(0x0000) - not-present page
[ 92.395365] PGD 0 P4D 0
[ 92.395566] Oops: 0000 [#1] PREEMPT SMP PTI
[ 92.395867] CPU: 2 PID: 506 Comm: mkfs.ext4 Not tainted 5.19.0-rc1+ #453
[ 92.396365] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
[ 92.397178] RIP: 0010:virtio_pmem_flush+0x2f/0x1f0
[ 92.397521] Code: 55 41 54 55 53 48 81 ec a0 00 00 00 65 48 8b 04
25 28 00 00 00 48 89 84 24 98 00 00 00 31 c0 48 8b 87 78 03 00 00 48
89 04 24 <48> 8b 98 18 03 00 00 e8 85 bf 6b 00 ba 58 00 00 00 be c0 0c
00 00
[ 92.398982] RSP: 0018:ffff9a7380aefc88 EFLAGS: 00010246
[ 92.399349] RAX: 0000000000000000 RBX: ffff8e77c3f86f00 RCX: 0000000000000000
[ 92.399833] RDX: ffffffffad4ea720 RSI: ffff8e77c41e39c0 RDI: ffff8e77c41c5c00
[ 92.400388] RBP: ffff8e77c41e39c0 R08: ffff8e77c19f0600 R09: 0000000000000000
[ 92.400874] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8e77c0814e28
[ 92.401364] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8e77c41e39c0
[ 92.401849] FS: 00007f3cd75b2780(0000) GS:ffff8e7937d00000(0000)
knlGS:0000000000000000
[ 92.402423] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 92.402821] CR2: 0000000000000318 CR3: 0000000103c80002 CR4: 0000000000370ee0
[ 92.403307] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 92.403793] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 92.404278] Call Trace:
[ 92.404481] <TASK>
[ 92.404654] ? mempool_alloc+0x5d/0x160
[ 92.404939] ? terminate_walk+0x5f/0xf0
[ 92.405226] ? bio_alloc_bioset+0xbb/0x3f0
[ 92.405525] async_pmem_flush+0x17/0x80
[ 92.405806] nvdimm_flush+0x11/0x30
[ 92.406067] pmem_submit_bio+0x1e9/0x200
[ 92.406354] __submit_bio+0x80/0x120
[ 92.406621] submit_bio_noacct_nocheck+0xdc/0x2a0
[ 92.406958] submit_bio_wait+0x4e/0x80
[ 92.407234] blkdev_issue_flush+0x31/0x50
[ 92.407526] ? punt_bios_to_rescuer+0x230/0x230
[ 92.407852] blkdev_fsync+0x1e/0x30
[ 92.408112] do_fsync+0x33/0x70
[ 92.408354] __x64_sys_fsync+0xb/0x10
[ 92.408625] do_syscall_64+0x43/0x90
[ 92.408895] entry_SYSCALL_64_after_hwframe+0x46/0xb0
[ 92.409257] RIP: 0033:0x7f3cd76c6c44
Fixes 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- Add calltrace to explain the issue in detail
---
drivers/nvdimm/virtio_pmem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
index 995b6cdc67ed..48f8327d0431 100644
--- a/drivers/nvdimm/virtio_pmem.c
+++ b/drivers/nvdimm/virtio_pmem.c
@@ -81,6 +81,7 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
ndr_desc.res = &res;
ndr_desc.numa_node = nid;
ndr_desc.flush = async_pmem_flush;
+ ndr_desc.provider_data = vdev;
set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
@@ -89,7 +90,6 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
err = -ENXIO;
goto out_nd;
}
- nd_region->provider_data = dev_to_virtio(nd_region->dev.parent->parent);
return 0;
out_nd:
nvdimm_bus_unregister(vpmem->nvdimm_bus);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH V2 2/2] virtio_pmem: set device ready in probe()
2022-06-27 6:29 [PATCH V2 1/2] virtio_pmem: initialize provider_data through nd_region_desc Jason Wang
@ 2022-06-27 6:29 ` Jason Wang
2022-06-27 7:59 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2022-06-27 6:29 UTC (permalink / raw)
To: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny, nvdimm,
linux-kernel, pankaj.gupta, mst
Cc: Jason Wang
The NVDIMM region could be available before the virtio_device_ready()
that is called by virtio_dev_probe(). This means the driver tries to
use device before DRIVER_OK which violates the spec, fixing this by
set device ready before the nvdimm_pmem_region_create().
Note that this means the virtio_pmem_host_ack() could be triggered
before the creation of the nd region, this is safe since the
virtio_pmem_host_ack() since pmem_lock has been initialized and
whether or not any available buffer is added before is validated.
Fixes 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since v1:
- Remove some comments per Dan
---
drivers/nvdimm/virtio_pmem.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
index 48f8327d0431..20da455d2ef6 100644
--- a/drivers/nvdimm/virtio_pmem.c
+++ b/drivers/nvdimm/virtio_pmem.c
@@ -84,6 +84,12 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
ndr_desc.provider_data = vdev;
set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
+ /*
+ * The NVDIMM region could be available before the
+ * virtio_device_ready() that is called by
+ * virtio_dev_probe(), so we set device ready here.
+ */
+ virtio_device_ready(vdev);
nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
if (!nd_region) {
dev_err(&vdev->dev, "failed to create nvdimm region\n");
@@ -92,6 +98,7 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
}
return 0;
out_nd:
+ virtio_reset_device(vdev);
nvdimm_bus_unregister(vpmem->nvdimm_bus);
out_vq:
vdev->config->del_vqs(vdev);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2 2/2] virtio_pmem: set device ready in probe()
2022-06-27 6:29 ` [PATCH V2 2/2] virtio_pmem: set device ready in probe() Jason Wang
@ 2022-06-27 7:59 ` Michael S. Tsirkin
2022-06-27 8:34 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-06-27 7:59 UTC (permalink / raw)
To: Jason Wang
Cc: dan.j.williams, vishal.l.verma, dave.jiang, ira.weiny, nvdimm,
linux-kernel, pankaj.gupta
On Mon, Jun 27, 2022 at 02:29:41PM +0800, Jason Wang wrote:
> The NVDIMM region could be available before the virtio_device_ready()
> that is called by virtio_dev_probe(). This means the driver tries to
> use device before DRIVER_OK which violates the spec, fixing this by
s/fixing this by/to fix this/
> set device ready before the nvdimm_pmem_region_create().
>
> Note that this means the virtio_pmem_host_ack() could be triggered
> before the creation of the nd region, this is safe since the
> virtio_pmem_host_ack() since pmem_lock has been initialized and
can't parse this sentence, since repeated twice confuses me
> whether or not any available buffer is added before is validated.
>
> Fixes 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes since v1:
> - Remove some comments per Dan
> ---
> drivers/nvdimm/virtio_pmem.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> index 48f8327d0431..20da455d2ef6 100644
> --- a/drivers/nvdimm/virtio_pmem.c
> +++ b/drivers/nvdimm/virtio_pmem.c
> @@ -84,6 +84,12 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
> ndr_desc.provider_data = vdev;
> set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + /*
> + * The NVDIMM region could be available before the
> + * virtio_device_ready() that is called by
> + * virtio_dev_probe(), so we set device ready here.
> + */
> + virtio_device_ready(vdev);
> nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> if (!nd_region) {
> dev_err(&vdev->dev, "failed to create nvdimm region\n");
> @@ -92,6 +98,7 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
> }
> return 0;
> out_nd:
> + virtio_reset_device(vdev);
> nvdimm_bus_unregister(vpmem->nvdimm_bus);
> out_vq:
> vdev->config->del_vqs(vdev);
> --
> 2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2 2/2] virtio_pmem: set device ready in probe()
2022-06-27 7:59 ` Michael S. Tsirkin
@ 2022-06-27 8:34 ` Jason Wang
2022-06-27 10:03 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2022-06-27 8:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Dan Williams, Vishal Verma, Jiang, Dave, Ira Weiny, Linux NVDIMM,
linux-kernel, pankaj.gupta
On Mon, Jun 27, 2022 at 4:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 27, 2022 at 02:29:41PM +0800, Jason Wang wrote:
> > The NVDIMM region could be available before the virtio_device_ready()
> > that is called by virtio_dev_probe(). This means the driver tries to
> > use device before DRIVER_OK which violates the spec, fixing this by
>
> s/fixing this by/to fix this/
>
> > set device ready before the nvdimm_pmem_region_create().
> >
> > Note that this means the virtio_pmem_host_ack() could be triggered
> > before the creation of the nd region, this is safe since the
> > virtio_pmem_host_ack() since pmem_lock has been initialized and
>
> can't parse this sentence, since repeated twice confuses me
Should be a copy-paste error: how about:
Note that this means the virtio_pmem_host_ack() could be triggered
before the creation of the nd region, this is safe since the pmem_lock
has been initialized and whether or not any available buffer is added
before is validated by virtio_pmem_host_ack().
Thanks
>
> > whether or not any available buffer is added before is validated.
> >
> > Fixes 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> > Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> > Changes since v1:
> > - Remove some comments per Dan
> > ---
> > drivers/nvdimm/virtio_pmem.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> > index 48f8327d0431..20da455d2ef6 100644
> > --- a/drivers/nvdimm/virtio_pmem.c
> > +++ b/drivers/nvdimm/virtio_pmem.c
> > @@ -84,6 +84,12 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
> > ndr_desc.provider_data = vdev;
> > set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + /*
> > + * The NVDIMM region could be available before the
> > + * virtio_device_ready() that is called by
> > + * virtio_dev_probe(), so we set device ready here.
> > + */
> > + virtio_device_ready(vdev);
> > nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> > if (!nd_region) {
> > dev_err(&vdev->dev, "failed to create nvdimm region\n");
> > @@ -92,6 +98,7 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
> > }
> > return 0;
> > out_nd:
> > + virtio_reset_device(vdev);
> > nvdimm_bus_unregister(vpmem->nvdimm_bus);
> > out_vq:
> > vdev->config->del_vqs(vdev);
> > --
> > 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2 2/2] virtio_pmem: set device ready in probe()
2022-06-27 8:34 ` Jason Wang
@ 2022-06-27 10:03 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-06-27 10:03 UTC (permalink / raw)
To: Jason Wang
Cc: Dan Williams, Vishal Verma, Jiang, Dave, Ira Weiny, Linux NVDIMM,
linux-kernel, pankaj.gupta
On Mon, Jun 27, 2022 at 04:34:07PM +0800, Jason Wang wrote:
> On Mon, Jun 27, 2022 at 4:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 27, 2022 at 02:29:41PM +0800, Jason Wang wrote:
> > > The NVDIMM region could be available before the virtio_device_ready()
> > > that is called by virtio_dev_probe(). This means the driver tries to
> > > use device before DRIVER_OK which violates the spec, fixing this by
> >
> > s/fixing this by/to fix this/
> >
> > > set device ready before the nvdimm_pmem_region_create().
> > >
> > > Note that this means the virtio_pmem_host_ack() could be triggered
> > > before the creation of the nd region, this is safe since the
> > > virtio_pmem_host_ack() since pmem_lock has been initialized and
> >
> > can't parse this sentence, since repeated twice confuses me
>
> Should be a copy-paste error: how about:
>
> Note that this means the virtio_pmem_host_ack() could be triggered
> before the creation of the nd region, this is safe since the pmem_lock
> has been initialized and whether or not any available buffer is added
> before is validated by virtio_pmem_host_ack().
>
> Thanks
looks good
> >
> > > whether or not any available buffer is added before is validated.
> > >
> > > Fixes 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> > > Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > Changes since v1:
> > > - Remove some comments per Dan
> > > ---
> > > drivers/nvdimm/virtio_pmem.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> > > index 48f8327d0431..20da455d2ef6 100644
> > > --- a/drivers/nvdimm/virtio_pmem.c
> > > +++ b/drivers/nvdimm/virtio_pmem.c
> > > @@ -84,6 +84,12 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
> > > ndr_desc.provider_data = vdev;
> > > set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > > set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > > + /*
> > > + * The NVDIMM region could be available before the
> > > + * virtio_device_ready() that is called by
> > > + * virtio_dev_probe(), so we set device ready here.
> > > + */
> > > + virtio_device_ready(vdev);
> > > nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
> > > if (!nd_region) {
> > > dev_err(&vdev->dev, "failed to create nvdimm region\n");
> > > @@ -92,6 +98,7 @@ static int virtio_pmem_probe(struct virtio_device *vdev)
> > > }
> > > return 0;
> > > out_nd:
> > > + virtio_reset_device(vdev);
> > > nvdimm_bus_unregister(vpmem->nvdimm_bus);
> > > out_vq:
> > > vdev->config->del_vqs(vdev);
> > > --
> > > 2.25.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-27 10:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 6:29 [PATCH V2 1/2] virtio_pmem: initialize provider_data through nd_region_desc Jason Wang
2022-06-27 6:29 ` [PATCH V2 2/2] virtio_pmem: set device ready in probe() Jason Wang
2022-06-27 7:59 ` Michael S. Tsirkin
2022-06-27 8:34 ` Jason Wang
2022-06-27 10:03 ` Michael S. Tsirkin
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.