From patchwork Wed May 16 15:23:43 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 10404207 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id EE63160155 for ; Wed, 16 May 2018 15:24:10 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DDC862842E for ; Wed, 16 May 2018 15:24:10 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D039C28434; Wed, 16 May 2018 15:24:10 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 618202842E for ; Wed, 16 May 2018 15:24:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752498AbeEPPX5 (ORCPT ); Wed, 16 May 2018 11:23:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57546 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752038AbeEPPXx (ORCPT ); Wed, 16 May 2018 11:23:53 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 85DAA3110751; Wed, 16 May 2018 15:23:53 +0000 (UTC) Received: from gimli.home (ovpn-116-135.phx2.redhat.com [10.3.116.135]) by smtp.corp.redhat.com (Postfix) with ESMTP id 485155D9C9; Wed, 16 May 2018 15:23:44 +0000 (UTC) Subject: [PATCH v2] vfio/mdev: Check globally for duplicate devices From: Alex Williamson To: kwankhede@nvidia.com Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, alex.williamson@redhat.com, cohuck@redhat.com Date: Wed, 16 May 2018 09:23:43 -0600 Message-ID: <20180516152228.12123.34883.stgit@gimli.home> User-Agent: StGit/0.18-102-gdf9f MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Wed, 16 May 2018 15:23:53 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP When we create an mdev device, we check for duplicates against the parent device and return -EEXIST if found, but the mdev device namespace is global since we'll link all devices from the bus. We do catch this later in sysfs_do_create_link_sd() to return -EEXIST, but with it comes a kernel warning and stack trace for trying to create duplicate sysfs links, which makes it an undesirable response. Therefore we should really be looking for duplicates across all mdev parent devices, or as implemented here, against our mdev device list. Notably, mdev_parent.lock really only seems to be serializing device creation and removal per parent. I'm not sure if this is necessary, mdev vendor drivers could easily provide this serialization if it is required, but a side-effect of holding the mdev_list_lock to protect the namespace is actually greater serialization across the create and remove paths, so mdev_parent.lock is removed. If we can show that vendor drivers handle the create/remove paths themselves, perhaps we can refine the locking granularity. Reviewed-by: Cornelia Huck Signed-off-by: Alex Williamson --- v2: Remove unnecessary ret init per Cornelia's review drivers/vfio/mdev/mdev_core.c | 77 +++++++++----------------------------- drivers/vfio/mdev/mdev_private.h | 1 2 files changed, 19 insertions(+), 59 deletions(-) diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 126991046eb7..aaab3ef93e1c 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -66,34 +66,6 @@ uuid_le mdev_uuid(struct mdev_device *mdev) } EXPORT_SYMBOL(mdev_uuid); -static int _find_mdev_device(struct device *dev, void *data) -{ - struct mdev_device *mdev; - - if (!dev_is_mdev(dev)) - return 0; - - mdev = to_mdev_device(dev); - - if (uuid_le_cmp(mdev->uuid, *(uuid_le *)data) == 0) - return 1; - - return 0; -} - -static bool mdev_device_exist(struct mdev_parent *parent, uuid_le uuid) -{ - struct device *dev; - - dev = device_find_child(parent->dev, &uuid, _find_mdev_device); - if (dev) { - put_device(dev); - return true; - } - - return false; -} - /* Should be called holding parent_list_lock */ static struct mdev_parent *__find_parent_device(struct device *dev) { @@ -221,7 +193,6 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops) } kref_init(&parent->ref); - mutex_init(&parent->lock); parent->dev = dev; parent->ops = ops; @@ -304,7 +275,7 @@ static void mdev_device_release(struct device *dev) int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid) { int ret; - struct mdev_device *mdev; + struct mdev_device *mdev, *tmp; struct mdev_parent *parent; struct mdev_type *type = to_mdev_type(kobj); @@ -312,12 +283,14 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid) if (!parent) return -EINVAL; - mutex_lock(&parent->lock); + mutex_lock(&mdev_list_lock); /* Check for duplicate */ - if (mdev_device_exist(parent, uuid)) { - ret = -EEXIST; - goto create_err; + list_for_each_entry(tmp, &mdev_list, next) { + if (!uuid_le_cmp(tmp->uuid, uuid)) { + ret = -EEXIST; + goto create_err; + } } mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); @@ -354,9 +327,6 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid) mdev->type_kobj = kobj; dev_dbg(&mdev->dev, "MDEV: created\n"); - mutex_unlock(&parent->lock); - - mutex_lock(&mdev_list_lock); list_add(&mdev->next, &mdev_list); mutex_unlock(&mdev_list_lock); @@ -366,7 +336,7 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, uuid_le uuid) device_unregister(&mdev->dev); create_err: - mutex_unlock(&parent->lock); + mutex_unlock(&mdev_list_lock); mdev_put_parent(parent); return ret; } @@ -382,6 +352,7 @@ int mdev_device_remove(struct device *dev, bool force_remove) mdev = to_mdev_device(dev); mutex_lock(&mdev_list_lock); + list_for_each_entry(tmp, &mdev_list, next) { if (tmp == mdev) { found = true; @@ -389,35 +360,25 @@ int mdev_device_remove(struct device *dev, bool force_remove) } } - if (found) - list_del(&mdev->next); - - mutex_unlock(&mdev_list_lock); - - if (!found) - return -ENODEV; + if (!found) { + ret = -ENODEV; + goto out; + } type = to_mdev_type(mdev->type_kobj); parent = mdev->parent; - mutex_lock(&parent->lock); ret = mdev_device_remove_ops(mdev, force_remove); - if (ret) { - mutex_unlock(&parent->lock); - - mutex_lock(&mdev_list_lock); - list_add(&mdev->next, &mdev_list); - mutex_unlock(&mdev_list_lock); - - return ret; - } + if (ret) + goto out; + list_del(&mdev->next); mdev_remove_sysfs_files(dev, type); device_unregister(dev); - mutex_unlock(&parent->lock); mdev_put_parent(parent); - - return 0; +out: + mutex_unlock(&mdev_list_lock); + return ret; } static int __init mdev_init(void) diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h index a9cefd70a705..85bb268c91be 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -20,7 +20,6 @@ struct mdev_parent { struct device *dev; const struct mdev_parent_ops *ops; struct kref ref; - struct mutex lock; struct list_head next; struct kset *mdev_types_kset; struct list_head type_list;