On Thu, Jun 23, 2022 at 11:57:26AM +0800, Jason Wang wrote: > On Thu, Jun 23, 2022 at 9:29 AM Jason Wang <jasow...@redhat.com> wrote: > > > > On Wed, Jun 22, 2022 at 8:31 PM Michael S. Tsirkin <m...@redhat.com> wrote: > > > > > > On Wed, Jun 22, 2022 at 03:24:15PM +0800, Jason Wang wrote: > > > > On Wed, Jun 22, 2022 at 2:29 PM Michael S. Tsirkin <m...@redhat.com> > > > > wrote: > > > > > > > > > > On Tue, Jun 21, 2022 at 03:38:35PM -0700, Dan Williams wrote: > > > > > > 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 > > > > > > > set device ready before the nvdimm_pmem_region_create(). > > > > > > > > > > > > Can you clarify the failure path. What race is virtio_device_ready() > > > > > > losing? > > > > > > > > > > > > > > > > > > > > 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 we > > > > > > > check if we've added any buffer before trying to proceed. > > > > > > > > > > > > I got a little bit lost with the usage of "we" here. Can you clarify > > > > > > which function / context is making which guarantee? > > > > > > > > > > > > > > > > > > > > Fixes 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver") > > > > > > > Signed-off-by: Jason Wang <jasow...@redhat.com> > > > > > > > --- > > > > > > > drivers/nvdimm/virtio_pmem.c | 12 ++++++++++++ > > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c > > > > > > > b/drivers/nvdimm/virtio_pmem.c > > > > > > > index 48f8327d0431..173f2f5adaea 100644 > > > > > > > --- a/drivers/nvdimm/virtio_pmem.c > > > > > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > > > > > @@ -84,6 +84,17 @@ 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. > > > > > > > + * > > > > > > > + * The callback - virtio_pmem_host_ack() is safe to be called > > > > > > > + * before the nvdimm_pmem_region_create() since the pmem_lock > > > > > > > + * has been initialized and legality of a used buffer is > > > > > > > + * validated before moving forward. > > > > > > > > > > > > This comment feels like changelog material. Just document why > > > > > > virtio_device_ready() must be called before device_add() of the > > > > > > nd_region. > > > > > > > > > > Agree here. More specifically if you are documenting why is it > > > > > safe to invoke each callback then that belongs to the callback itself. > > > > > > > > Ok, so I will move it to the callback and leave a simple comment like > > > > > > > > " See comment in virtio_pmem_host_ack(), it is safe to be called > > > > before nvdimm_pmem_region_create()" > > > > > > > > Thanks > > > > > > No, just document why virtio_device_ready() must be called before > > > device_add() > > > > > > I don't think the idea of working around these issues by adding code > > > to virtio_device_ready worked so far, > > > > Any issue you found in this approach? > > > > > not at all sure this approach > > > is here to stay. > > > > Or do you have other ideas to fix this issue? > > Or do you think we can do something similar to harden the config > interrupt (down the road with the kconfig option)? > > virtio_device_ready(); // set driver ok but delay the vring interrupt > subsystem_register(); > virtio_enable_vq_callback(); // enable vring interrupt and raised > delayed interrupt > > Thanks
Yes and from API POV I think we should do virtio_disable_vq_callback(); virtio_device_ready(); subsystem_register(); virtio_enable_vq_callback(); this way we won't break all drivers that aren't careful like previous hardening patches did. > > > > Thanks > > > > > > > > > > > > > > > > > > > > + */ > > > > > > > + 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 +103,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 > > > > > > > > > > > > > > >