Messages in this thread | | | Subject | Re: [PATCH v4 0/2] vfio/mdev: Device namespace protection | From | Halil Pasic <> | Date | Tue, 22 May 2018 19:17:07 +0200 |
| |
From vfio-ccw perspective I join Connie's assessment: vfio-ccw should be fine with these changes. I'm however not too deeply involved with the mdev framework, thus I don't feel comfortable r-b-ing. That results in Acked-by: Halil Pasic <pasic@linux.ibm.com> for both patches.
While at it I have would like to ask about the semantics and intended use of the mdev interfaces.
static int vfio_ccw_sch_probe(struct subchannel *sch) {
/* HALIL: 8< Not so interesting stuff happens here. >8 */ ret = vfio_ccw_mdev_reg(sch); if (ret) goto out_disable; /* * HALIL: * This might be racy. Somewhere in vfio_ccw_mdev_reg() the create attribute * is made available (it calls mdev_register_device()). For instance create will * attempt to decrement private->avail which is initialized below. I fail to * understand how is this well synchronized. */ INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo); atomic_set(&private->avail, 1); private->state = VFIO_CCW_STATE_STANDBY;
return 0;
out_disable: cio_disable_subchannel(sch); out_free: dev_set_drvdata(&sch->dev, NULL); kfree(private); return ret; }
Should not initialization of go before mdev_register_device(), and then rolled back if necessary if mdev_register_device() fails?
In practice it does not seem very likely that userspace can trigger mdev_device_create() before vfio_ccw_sch_probe() finishes so it should not be a practical problem. But I would like to understand how synchronization is supposed to work.
[Added Dong Jia, maybe he is also able to answer my question.]
Regards, Halil
On 05/18/2018 09:10 PM, Alex Williamson wrote: > v4: Fix the 'create' racing 'remove' gap noted by Kirti by moving > removal from mdev_list to mdev_device_release(). Fix missing > mdev_put_parent() cases in mdev_device_create(), also noted > by Kirti. Added documention update regarding serialization as > noted by Cornelia. Added additional commit log comment about > -EAGAIN vs -ENODEV for 'remove' racing 'create'. Added second > patch to re-order sysfs attributes, with this my targeted > scripts can no longer hit the gap where -EAGAIN is regurned. > BTW, the gap where the current code returns -ENODEV in this > race condition is about 50% easier to hit than it exists in > this series with patch 1 alone. > > Thanks, > Alex > > --- > > Alex Williamson (2): > vfio/mdev: Check globally for duplicate devices > vfio/mdev: Re-order sysfs attribute creation > > > Documentation/vfio-mediated-device.txt | 5 ++ > drivers/vfio/mdev/mdev_core.c | 102 +++++++++++--------------------- > drivers/vfio/mdev/mdev_private.h | 2 - > drivers/vfio/mdev/mdev_sysfs.c | 14 ++-- > 4 files changed, 49 insertions(+), 74 deletions(-) >
| |