[PATCH v3 0/2] avoid inserting duplicate IDs in dynids list

[PATCH v3 0/2] avoid inserting duplicate IDs in dynids list

[Zhenzhong Duan]

vfio-pci and pci-stub use new_id to bind devices. But one can add same IDs
multiple times, for example:

# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
-bash: echo: write error: No such device

This doesn't cause user-visible broken behavior, but not user friendly.
he has to remove same IDs same times to ensure it's completely gone.

Changed to only allow one dynamic entry of the same kind, after fix:

# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
-bash: echo: write error: File exists
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
# echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
-bash: echo: write error: No such device


v3: add a separate patch to process dependency issue per Bjorn
    make commit log more clear per Bjorn
v2: revert the export of pci_match_device() per Christoph
    combind PATCH1 and PATCH2 into one.

v2 link:https://lkml.org/lkml/2020/10/25/347

Zhenzhong Duan (2):
  PCI: move pci_match_device() ahead of new_id_store()
  PCI: avoid duplicate IDs in dynamic IDs list

 drivers/pci/pci-driver.c | 146 +++++++++++++++++++++++------------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

-- 
1.8.3.1


Zhenzhong Duan (2):
  PCI: move pci_match_device() ahead of new_id_store()
  PCI: avoid duplicate IDs in dynamic IDs list

 drivers/pci/pci-driver.c | 146 +++++++++++++++++++++++------------------------
 1 file changed, 73 insertions(+), 73 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/2] PCI: move pci_match_device() ahead of new_id_store()

[Zhenzhong Duan]

Move pci_match_device() and it's dependencies (pci_match_id() and
pci_device_id_any) ahead of new_id_store().

This is preparation work for calling pci_match_device() in new_id_store().
No functional changes.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
---
 drivers/pci/pci-driver.c | 144 +++++++++++++++++++++++------------------------
 1 file changed, 72 insertions(+), 72 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 8b587fc..e928cfa 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -90,6 +90,78 @@ static void pci_free_dynids(struct pci_driver *drv)
 }
 
 /**
+ * pci_match_id - See if a pci device matches a given pci_id table
+ * @ids: array of PCI device id structures to search in
+ * @dev: the PCI device structure to match against.
+ *
+ * Used by a driver to check whether a PCI device present in the
+ * system is in its list of supported devices.  Returns the matching
+ * pci_device_id structure or %NULL if there is no match.
+ *
+ * Deprecated, don't use this as it will not catch any dynamic ids
+ * that a driver might want to check for.
+ */
+const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
+					 struct pci_dev *dev)
+{
+	if (ids) {
+		while (ids->vendor || ids->subvendor || ids->class_mask) {
+			if (pci_match_one_device(ids, dev))
+				return ids;
+			ids++;
+		}
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(pci_match_id);
+
+static const struct pci_device_id pci_device_id_any = {
+	.vendor = PCI_ANY_ID,
+	.device = PCI_ANY_ID,
+	.subvendor = PCI_ANY_ID,
+	.subdevice = PCI_ANY_ID,
+};
+
+/**
+ * pci_match_device - Tell if a PCI device structure has a matching PCI device id structure
+ * @drv: the PCI driver to match against
+ * @dev: the PCI device structure to match against
+ *
+ * Used by a driver to check whether a PCI device present in the
+ * system is in its list of supported devices.  Returns the matching
+ * pci_device_id structure or %NULL if there is no match.
+ */
+static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
+						    struct pci_dev *dev)
+{
+	struct pci_dynid *dynid;
+	const struct pci_device_id *found_id = NULL;
+
+	/* When driver_override is set, only bind to the matching driver */
+	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
+		return NULL;
+
+	/* Look at the dynamic ids first, before the static ones */
+	spin_lock(&drv->dynids.lock);
+	list_for_each_entry(dynid, &drv->dynids.list, node) {
+		if (pci_match_one_device(&dynid->id, dev)) {
+			found_id = &dynid->id;
+			break;
+		}
+	}
+	spin_unlock(&drv->dynids.lock);
+
+	if (!found_id)
+		found_id = pci_match_id(drv->id_table, dev);
+
+	/* driver_override will always match, send a dummy id */
+	if (!found_id && dev->driver_override)
+		found_id = &pci_device_id_any;
+
+	return found_id;
+}
+
+/**
  * store_new_id - sysfs frontend to pci_add_dynid()
  * @driver: target device driver
  * @buf: buffer for scanning device ID data
@@ -208,78 +280,6 @@ static ssize_t remove_id_store(struct device_driver *driver, const char *buf,
 };
 ATTRIBUTE_GROUPS(pci_drv);
 
-/**
- * pci_match_id - See if a pci device matches a given pci_id table
- * @ids: array of PCI device id structures to search in
- * @dev: the PCI device structure to match against.
- *
- * Used by a driver to check whether a PCI device present in the
- * system is in its list of supported devices.  Returns the matching
- * pci_device_id structure or %NULL if there is no match.
- *
- * Deprecated, don't use this as it will not catch any dynamic ids
- * that a driver might want to check for.
- */
-const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
-					 struct pci_dev *dev)
-{
-	if (ids) {
-		while (ids->vendor || ids->subvendor || ids->class_mask) {
-			if (pci_match_one_device(ids, dev))
-				return ids;
-			ids++;
-		}
-	}
-	return NULL;
-}
-EXPORT_SYMBOL(pci_match_id);
-
-static const struct pci_device_id pci_device_id_any = {
-	.vendor = PCI_ANY_ID,
-	.device = PCI_ANY_ID,
-	.subvendor = PCI_ANY_ID,
-	.subdevice = PCI_ANY_ID,
-};
-
-/**
- * pci_match_device - Tell if a PCI device structure has a matching PCI device id structure
- * @drv: the PCI driver to match against
- * @dev: the PCI device structure to match against
- *
- * Used by a driver to check whether a PCI device present in the
- * system is in its list of supported devices.  Returns the matching
- * pci_device_id structure or %NULL if there is no match.
- */
-static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
-						    struct pci_dev *dev)
-{
-	struct pci_dynid *dynid;
-	const struct pci_device_id *found_id = NULL;
-
-	/* When driver_override is set, only bind to the matching driver */
-	if (dev->driver_override && strcmp(dev->driver_override, drv->name))
-		return NULL;
-
-	/* Look at the dynamic ids first, before the static ones */
-	spin_lock(&drv->dynids.lock);
-	list_for_each_entry(dynid, &drv->dynids.list, node) {
-		if (pci_match_one_device(&dynid->id, dev)) {
-			found_id = &dynid->id;
-			break;
-		}
-	}
-	spin_unlock(&drv->dynids.lock);
-
-	if (!found_id)
-		found_id = pci_match_id(drv->id_table, dev);
-
-	/* driver_override will always match, send a dummy id */
-	if (!found_id && dev->driver_override)
-		found_id = &pci_device_id_any;
-
-	return found_id;
-}
-
 struct drv_dev_and_id {
 	struct pci_driver *drv;
 	struct pci_dev *dev;
-- 
1.8.3.1

[PATCH v3 2/2] PCI: avoid duplicate IDs in dynamic IDs list

[Zhenzhong Duan]

When a device ID data is writen to /sys/bus/pci/drivers/.../new_id,
only static ID table is checked for duplicate and multiple dynamic ID
entries of same kind are allowed to exist in a dynamic IDs list.

This doesn't cause user-visible broken behavior, but not user friendly.
remove_id_store() only remove one of the duplicate IDs, so if we add an
ID several times, we would have to remove it the same number of times
before it's completely gone.

Fix it by calling pci_match_device() which checks both dynamic and static
IDs to avoid inserting duplicate IDs in dynamic IDs list.

After fix, it shows below result which is expected:

echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
-bash: echo: write error: File exists

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@gmail.com>
---
 drivers/pci/pci-driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index e928cfa..c4678d8 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -197,7 +197,7 @@ static ssize_t new_id_store(struct device_driver *driver, const char *buf,
 		pdev->subsystem_device = subdevice;
 		pdev->class = class;
 
-		if (pci_match_id(pdrv->id_table, pdev))
+		if (pci_match_device(pdrv, pdev))
 			retval = -EEXIST;
 
 		kfree(pdev);
-- 
1.8.3.1

Re: [PATCH v3 0/2] avoid inserting duplicate IDs in dynids list

[Bjorn Helgaas]

On Tue, Nov 17, 2020 at 01:44:07PM +0800, Zhenzhong Duan wrote:
> vfio-pci and pci-stub use new_id to bind devices. But one can add same IDs
> multiple times, for example:
> 
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
> -bash: echo: write error: No such device
> 
> This doesn't cause user-visible broken behavior, but not user friendly.
> he has to remove same IDs same times to ensure it's completely gone.
> 
> Changed to only allow one dynamic entry of the same kind, after fix:
> 
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/new_id
> -bash: echo: write error: File exists
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
> # echo "1af4 1041" > /sys/bus/pci/drivers/vfio-pci/remove_id
> -bash: echo: write error: No such device
> 
> 
> v3: add a separate patch to process dependency issue per Bjorn
>     make commit log more clear per Bjorn
> v2: revert the export of pci_match_device() per Christoph
>     combind PATCH1 and PATCH2 into one.
> 
> v2 link:https://lkml.org/lkml/2020/10/25/347
> 
> Zhenzhong Duan (2):
>   PCI: move pci_match_device() ahead of new_id_store()
>   PCI: avoid duplicate IDs in dynamic IDs list
> 
>  drivers/pci/pci-driver.c | 146 +++++++++++++++++++++++------------------------
>  1 file changed, 73 insertions(+), 73 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 
> Zhenzhong Duan (2):
>   PCI: move pci_match_device() ahead of new_id_store()
>   PCI: avoid duplicate IDs in dynamic IDs list
> 
>  drivers/pci/pci-driver.c | 146 +++++++++++++++++++++++------------------------
>  1 file changed, 73 insertions(+), 73 deletions(-)

I corrected the subject lines:

  PCI: Move pci_match_device() ahead of new_id_store()
  PCI: Avoid duplicate IDs in driver dynamic IDs list

and applied both to pci/enumeration for v5.11, thanks!