From mboxrd@z Thu Jan 1 00:00:00 1970 From: Halil Pasic Subject: Re: [PATCH v4 0/2] vfio/mdev: Device namespace protection Date: Tue, 22 May 2018 19:17:07 +0200 Message-ID: References: <20180518190145.3187.7620.stgit@gimli.home> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, cohuck@redhat.com To: Alex Williamson , kwankhede@nvidia.com, Dong Jia Return-path: In-Reply-To: <20180518190145.3187.7620.stgit@gimli.home> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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 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(-) >