RE: [PATCH 8/8] vfio/mdev: Improve the create/remove sequence

From: Parav Pandit
Date: Mon Mar 25 2019 - 23:19:26 EST



> -----Original Message-----
> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Monday, March 25, 2019 9:17 PM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kwankhede@xxxxxxxxxx
> Subject: Re: [PATCH 8/8] vfio/mdev: Improve the create/remove sequence
>
> On Tue, 26 Mar 2019 01:43:44 +0000
> Parav Pandit <parav@xxxxxxxxxxxx> wrote:
>
> > > -----Original Message-----
> > > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > Sent: Monday, March 25, 2019 7:06 PM
> > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > kwankhede@xxxxxxxxxx
> > > Subject: Re: [PATCH 8/8] vfio/mdev: Improve the create/remove
> > > sequence
> > >
> > > On Mon, 25 Mar 2019 23:34:28 +0000
> > > Parav Pandit <parav@xxxxxxxxxxxx> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > > > Sent: Monday, March 25, 2019 6:19 PM
> > > > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > > > kwankhede@xxxxxxxxxx
> > > > > Subject: Re: [PATCH 8/8] vfio/mdev: Improve the create/remove
> > > > > sequence
> > > > >
> > > > > On Fri, 22 Mar 2019 18:20:35 -0500 Parav Pandit
> > > > > <parav@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > > There are five problems with current code structure.
> > > > > > 1. mdev device is placed on the mdev bus before it is created
> > > > > > in the vendor driver. Once a device is placed on the mdev bus
> > > > > > without creating its supporting underlying vendor device, an
> > > > > > open() can get triggered by userspace on partially initialized device.
> > > > > > Below ladder diagram highlight it.
> > > > > >
> > > > > > cpu-0 cpu-1
> > > > > > ----- -----
> > > > > > create_store()
> > > > > > mdev_create_device()
> > > > > > device_register()
> > > > > > ...
> > > > > > vfio_mdev_probe()
> > > > > > ...creates char device
> > > > > > vfio_mdev_open()
> > > > > > parent->ops->open(mdev)
> > > > > > vfio_ap_mdev_open()
> > > > > > matrix_mdev = NULL
> > > > > > [...]
> > > > > > parent->ops->create()
> > > > > > vfio_ap_mdev_create()
> > > > > > mdev_set_drvdata(mdev, matrix_mdev);
> > > > > > /* Valid pointer set above */
> > > > > >
> > > > > > 2. Current creation sequence is,
> > > > > > parent->ops_create()
> > > > > > groups_register()
> > > > > >
> > > > > > Remove sequence is,
> > > > > > parent->ops->remove()
> > > > > > groups_unregister()
> > > > > > However, remove sequence should be exact mirror of creation
> > > sequence.
> > > > > > Once this is achieved, all users of the mdev will be
> > > > > > terminated first before removing underlying vendor device.
> > > > > > (Follow standard linux driver model).
> > > > > > At that point vendor's remove() ops shouldn't failed because
> > > > > > device is taken off the bus that should terminate the users.
> > > > > >
> > > > > > 3. Additionally any new mdev driver that wants to work on mdev
> > > > > > device during probe() routine registered using
> > > > > > mdev_register_driver() needs to get stable mdev structure.
> > > > > >
> > > > > > 4. In following sequence, child devices created while removing
> > > > > > mdev parent device can be left out, or it may lead to race of
> > > > > > removing half initialized child mdev devices.
> > > > > >
> > > > > > issue-1:
> > > > > > --------
> > > > > > cpu-0 cpu-1
> > > > > > ----- -----
> > > > > > mdev_unregister_device()
> > > > > > device_for_each_child()
> > > > > > mdev_device_remove_cb()
> > > > > >
> > > > > > mdev_device_remove()
> > > > > > create_store()
> > > > > > mdev_device_create() [...]
> > > > > > device_register()
> > > > > > parent_remove_sysfs_files()
> > > > > > /* BUG: device added by cpu-0
> > > > > > * whose parent is getting removed.
> > > > > > */
> > > > > >
> > > > > > issue-2:
> > > > > > --------
> > > > > > cpu-0 cpu-1
> > > > > > ----- -----
> > > > > > create_store()
> > > > > > mdev_device_create() [...]
> > > > > > device_register()
> > > > > >
> > > > > > [...] mdev_unregister_device()
> > > > > > device_for_each_child()
> > > > > > mdev_device_remove_cb()
> > > > > >
> > > > > > mdev_device_remove()
> > > > > >
> > > > > > mdev_create_sysfs_files()
> > > > > > /* BUG: create is adding
> > > > > > * sysfs files for a device
> > > > > > * which is undergoing removal.
> > > > > > */
> > > > > > parent_remove_sysfs_files()
> > > > >
> > > > > In both cases above, it looks like the device will hold a
> > > > > reference to the parent, so while there is a race, the parent object
> isn't released.
> > > > Yes, parent object is not released but parent fields are not stable.
> > > >
> > > > >
> > > > > >
> > > > > > 5. Below crash is observed when user initiated remove is in
> > > > > > progress and mdev_unregister_driver() completes parent
> > > unregistration.
> > > > > >
> > > > > > cpu-0 cpu-1
> > > > > > ----- -----
> > > > > > remove_store()
> > > > > > mdev_device_remove()
> > > > > > active = false;
> > > > > > mdev_unregister_device()
> > > > > > remove type
> > > > > > [...]
> > > > > > mdev_remove_ops() crashes.
> > > > > >
> > > > > > This is similar race like create() racing with
> mdev_unregister_device().
> > > > >
> > > > > Not sure I catch this, the device should have a reference to the
> > > > > parent, and we don't specifically clear parent->ops, so what's
> > > > > getting removed that causes this oops? Is .remove pointing at
> > > > > bad text
> > > regardless?
> > > > >
> > > > I guess the mdev_attr_groups being stale now.
> > > >
> > > > > > mtty mtty: MDEV: Registered
> > > > > > iommu: Adding device 83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 to
> > > > > > group
> > > > > > 57 vfio_mdev 83b8f4f2-509f-382f-3c1e-e6bfe0fa1001: MDEV:
> > > > > > group_id = 57 mdev_device_remove sleep started mtty mtty: MDEV:
> > > > > > Unregistering
> > > > > > mtty_dev: Unloaded!
> > > > > > BUG: unable to handle kernel paging request at
> > > > > > ffffffffc027d668 PGD
> > > > > > af9818067 P4D af9818067 PUD af981a067 PMD 8583c3067 PTE 0
> > > > > > Oops: 0000 [#1] SMP PTI
> > > > > > CPU: 15 PID: 3517 Comm: bash Kdump: loaded Not tainted
> > > > > > 5.0.0-rc7-vdevbus+ #2 Hardware name: Supermicro
> > > > > > SYS-6028U-TR4+/X10DRU-i+, BIOS 2.0b 08/09/2016
> > > > > > RIP: 0010:mdev_device_remove_ops+0x1a/0x50 [mdev] Call Trace:
> > > > > > mdev_device_remove+0xef/0x130 [mdev]
> > > > > > remove_store+0x77/0xa0 [mdev]
> > > > > > kernfs_fop_write+0x113/0x1a0
> > > > > > __vfs_write+0x33/0x1b0
> > > > > > ? rcu_read_lock_sched_held+0x64/0x70
> > > > > > ? rcu_sync_lockdep_assert+0x2a/0x50 ?
> > > > > > __sb_start_write+0x121/0x1b0 ? vfs_write+0x17c/0x1b0
> > > > > > vfs_write+0xad/0x1b0
> > > > > > ? trace_hardirqs_on_thunk+0x1a/0x1c
> > > > > > ksys_write+0x55/0xc0
> > > > > > do_syscall_64+0x5a/0x210
> > > > > >
> > > > > > Therefore, mdev core is improved in following ways to overcome
> > > > > > above issues.
> > > > > >
> > > > > > 1. Before placing mdev devices on the bus, perform vendor
> > > > > > drivers creation which supports the mdev creation.
> > > > > > This ensures that mdev specific all necessary fields are
> > > > > > initialized before a given mdev can be accessed by bus driver.
> > > > > >
> > > > > > 2. During remove flow, first remove the device from the bus.
> > > > > > This ensures that any bus specific devices and data is cleared.
> > > > > > Once device is taken of the mdev bus, perform remove() of mdev
> > > > > > from the vendor driver.
> > > > > >
> > > > > > 3. Linux core device model provides way to register and auto
> > > > > > unregister the device sysfs attribute groups at dev->groups.
> > > > > > Make use of this groups to let core create the groups and
> > > > > > simplify code to avoid explicit groups creation and removal.
> > > > > >
> > > > > > 4. Wait for any ongoing mdev create() and remove() to finish
> > > > > > before unregistering parent device using srcu. This continues
> > > > > > to allow multiple create and remove to progress in parallel.
> > > > > > At the same time guard parent removal while parent is being
> > > > > > access by
> > > > > > create() and remove
> > > > > callbacks.
> > > > >
> > > > > So there should be 4-5 separate patches here? Wishful thinking?
> > > > >
> > > > create, remove racing with unregister is handled using srcu.
> > > > Change-3 cannot be done without fixing the sequence so it should
> > > > be in
> > > patch that fixes it.
> > > > Change described changes 1-2-3 are just one change. It is just the
> > > > patch
> > > description to bring clarity.
> > > > Change-4 can be possibly done as split to different patch.
> > > >
> > > > > > Fixes: 7b96953bc640 ("vfio: Mediated device Core driver")
> > > > > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/vfio/mdev/mdev_core.c | 142 +++++++++++++++++++++----
> -----
> > > -----
> > > > > ----
> > > > > > drivers/vfio/mdev/mdev_private.h | 7 +-
> > > > > > drivers/vfio/mdev/mdev_sysfs.c | 6 +-
> > > > > > 3 files changed, 84 insertions(+), 71 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c
> > > > > > b/drivers/vfio/mdev/mdev_core.c index 944a058..8fe0ed1 100644
> > > > > > --- a/drivers/vfio/mdev/mdev_core.c
> > > > > > +++ b/drivers/vfio/mdev/mdev_core.c
> > > > > > @@ -84,6 +84,7 @@ static void mdev_release_parent(struct kref
> *kref)
> > > > > > ref);
> > > > > > struct device *dev = parent->dev;
> > > > > >
> > > > > > + cleanup_srcu_struct(&parent->unreg_srcu);
> > > > > > kfree(parent);
> > > > > > put_device(dev);
> > > > > > }
> > > > > > @@ -103,56 +104,30 @@ static inline void
> > > > > > mdev_put_parent(struct
> > > > > mdev_parent *parent)
> > > > > > kref_put(&parent->ref, mdev_release_parent); }
> > > > > >
> > > > > > -static int mdev_device_create_ops(struct kobject *kobj,
> > > > > > - struct mdev_device *mdev)
> > > > > > +static int mdev_device_must_remove(struct mdev_device *mdev)
> > > > >
> > > > > Naming is off here, mdev_device_remove_common()?
> > > > >
> > > > Yes, sounds better.
> > > >
> > > > > > {
> > > > > > - struct mdev_parent *parent = mdev->parent;
> > > > > > + struct mdev_parent *parent;
> > > > > > + struct mdev_type *type;
> > > > > > int ret;
> > > > > >
> > > > > > - ret = parent->ops->create(kobj, mdev);
> > > > > > - if (ret)
> > > > > > - return ret;
> > > > > > + type = to_mdev_type(mdev->type_kobj);
> > > > > >
> > > > > > - ret = sysfs_create_groups(&mdev->dev.kobj,
> > > > > > - parent->ops->mdev_attr_groups);
> > > > > > + mdev_remove_sysfs_files(&mdev->dev, type);
> > > > > > + device_del(&mdev->dev);
> > > > > > + parent = mdev->parent;
> > > > > > + ret = parent->ops->remove(mdev);
> > > > > > if (ret)
> > > > > > - parent->ops->remove(mdev);
> > > > > > + dev_err(&mdev->dev, "Remove failed: err=%d\n",
> ret);
> > > > >
> > > > > Let the caller decide whether to be verbose with the error,
> > > > > parent removal might want to warn, sysfs remove might just return
> an error.
> > > > >
> > > > I didn't follow. Caller meaning mdev_device_remove_common() or
> > > > vendor
> > > driver?
> > >
> > > I mean the callback iterator on the parent remove can do a WARN_ON
> > > if this returns an error while the device remove path can silently
> > > return -EBUSY, the common function doesn't need to decide whether
> > > the parent ops remove function deserves a dev_err.
> > >
> > Ok. I understood.
> > But device remove returning silent -EBUSY looks an error that should
> > get logged in, because this is something not expected. Its probably
> > late for sysfs layer to return report an error by that time it prints
> > device name, because put_device() is done. So if remove() returns an
> > error, I think its legitimate failure to do WARN_ON or dev_err().
>
> Calling put_device() if the parent remove op fails looks like a bug introduced
> by this series, the current code allows that failure leaving the device in a
> coherent state and returning errno to the sysfs store function.
>
Why should it fail?
We are taking off the device bus first as describe in commit log.
This ensures that everything is closed before calling the remove().
We cannot avoid put_device() and put_parent, it all buggy path...