[PATCH] PCI: rework new_id interface for known vendor/device values

[PATCH] PCI: rework new_id interface for known vendor/device values

[Bandan Das]

While using the new_id interface, the user can unintentionally feed
incorrect values if the driver static table has a matching entry.
This is possible since only the device and vendor fields are
mandatory and the rest are optional. As a result, store_new_id
will fill in default values that are then passed on to the driver
and can have unintended consequences.

As an example, consider the ixgbe driver and the 82599EB network card :
echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id

This will pass a driver_data value of 0 to the driver whereas
the index 0 in ixgbe actually points to a different set of card
operations.

This change automatically selects the matching static entry if there
is one for the newly created dynid. However, if the user intentionally
wants a different set of values, she must provide all the 7 fields
and the static entry will be ignored.

In most cases, this use case seems unnecessary, however, this
is a common libvirt/KVM/device assignment scenario where the
user might want to bind a device back to the host driver.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 25f0bc6..187e572 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -90,6 +90,24 @@ static void pci_free_dynids(struct pci_driver *drv)
 	spin_unlock(&drv->dynids.lock);
 }
 
+static const struct
+pci_device_id *match_id_table_entry(struct device_driver *driver,
+				    __u32 vendor, __u32 device)
+{
+	struct pci_driver *pdrv = to_pci_driver(driver);
+	const struct pci_device_id *ids = pdrv->id_table;
+
+	if (ids) {
+		while (ids->vendor || ids->subvendor || ids->class_mask) {
+			if ((ids->vendor == vendor) && (ids->device == device))
+				return ids;
+			ids++;
+		}
+	}
+
+	return NULL;
+}
+
 /**
  * store_new_id - sysfs frontend to pci_add_dynid()
  * @driver: target device driver
@@ -102,7 +120,8 @@ static ssize_t
 store_new_id(struct device_driver *driver, const char *buf, size_t count)
 {
 	struct pci_driver *pdrv = to_pci_driver(driver);
-	const struct pci_device_id *ids = pdrv->id_table;
+	const struct pci_device_id *ids = pdrv->id_table,
+		*tids = NULL;
 	__u32 vendor, device, subvendor=PCI_ANY_ID,
 		subdevice=PCI_ANY_ID, class=0, class_mask=0;
 	unsigned long driver_data=0;
@@ -115,9 +134,24 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
 	if (fields < 2)
 		return -EINVAL;
 
-	/* Only accept driver_data values that match an existing id_table
-	   entry */
-	if (ids) {
+	tids = match_id_table_entry(driver, vendor, device);
+
+	if (tids && (fields != 7)) {
+
+		subvendor = tids->subvendor;
+		subdevice = tids->subdevice;
+		class = tids->class;
+		class_mask = tids->class_mask;
+		driver_data = tids->driver_data;
+
+		pr_warn("pci: Using driver (%s) static DeviceID table entry for vendor 0x%04x and device 0x%04x",
+			driver->name, vendor, device);
+
+	} else if (ids) {
+
+		/* Only accept driver_data values that match an existing
+		   id_table entry */
+
 		retval = -EINVAL;
 		while (ids->vendor || ids->subvendor || ids->class_mask) {
 			if (driver_data == ids->driver_data) {
-- 
1.8.3.1

Re: [PATCH] PCI: rework new_id interface for known vendor/device values

[Alex Williamson]

On Mon, 2014-03-31 at 00:28 -0400, Bandan Das wrote:
> While using the new_id interface, the user can unintentionally feed
> incorrect values if the driver static table has a matching entry.
> This is possible since only the device and vendor fields are
> mandatory and the rest are optional. As a result, store_new_id
> will fill in default values that are then passed on to the driver
> and can have unintended consequences.
> 
> As an example, consider the ixgbe driver and the 82599EB network card :
> echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id
> 
> This will pass a driver_data value of 0 to the driver whereas
> the index 0 in ixgbe actually points to a different set of card
> operations.
> 
> This change automatically selects the matching static entry if there
> is one for the newly created dynid. However, if the user intentionally
> wants a different set of values, she must provide all the 7 fields
> and the static entry will be ignored.
> 
> In most cases, this use case seems unnecessary, however, this
> is a common libvirt/KVM/device assignment scenario where the
> user might want to bind a device back to the host driver.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 25f0bc6..187e572 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -90,6 +90,24 @@ static void pci_free_dynids(struct pci_driver *drv)
>  	spin_unlock(&drv->dynids.lock);
>  }
>  
> +static const struct
> +pci_device_id *match_id_table_entry(struct device_driver *driver,
> +				    __u32 vendor, __u32 device)
> +{
> +	struct pci_driver *pdrv = to_pci_driver(driver);
> +	const struct pci_device_id *ids = pdrv->id_table;
> +
> +	if (ids) {
> +		while (ids->vendor || ids->subvendor || ids->class_mask) {
> +			if ((ids->vendor == vendor) && (ids->device == device))
> +				return ids;
> +			ids++;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * store_new_id - sysfs frontend to pci_add_dynid()
>   * @driver: target device driver
> @@ -102,7 +120,8 @@ static ssize_t
>  store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  {
>  	struct pci_driver *pdrv = to_pci_driver(driver);
> -	const struct pci_device_id *ids = pdrv->id_table;
> +	const struct pci_device_id *ids = pdrv->id_table,
> +		*tids = NULL;
>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>  	unsigned long driver_data=0;
> @@ -115,9 +134,24 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>  	if (fields < 2)
>  		return -EINVAL;
>  
> -	/* Only accept driver_data values that match an existing id_table
> -	   entry */
> -	if (ids) {
> +	tids = match_id_table_entry(driver, vendor, device);
> +

Would it make more sense to construct a pci_dev, ex:

if (fields != 7) {
	struct pci_dev dev = { .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID };

	dev.vendor = vendor;
	dev.device = device;
	if (fields > 2)
		dev.subvendor = subvendor;
	if (fields > 3)
		dev.subdevice = subdevice;
	...

	if (pci_match_id(drv->id_table, &dev))
		return -EEXIST;
}


> +	if (tids && (fields != 7)) {
> +
> +		subvendor = tids->subvendor;
> +		subdevice = tids->subdevice;
> +		class = tids->class;
> +		class_mask = tids->class_mask;
> +		driver_data = tids->driver_data;

This doesn't look right.  First, we're potentially overwriting user
stored data for fields >2 but <7.  Second, we only matched on vendor &
device and could be filling the rest with data that isn't the best match
(and is guaranteed to just be a duplicate of a static table ID).

> +
> +		pr_warn("pci: Using driver (%s) static DeviceID table entry for vendor 0x%04x and device 0x%04x",
> +			driver->name, vendor, device);

I think we should be error'ing rather than inventing a duplicate ID to
insert.  How would a user ever know how to use remove_id to clean out
this new_id?  Thanks,

Alex

> +
> +	} else if (ids) {
> +
> +		/* Only accept driver_data values that match an existing
> +		   id_table entry */
> +
>  		retval = -EINVAL;
>  		while (ids->vendor || ids->subvendor || ids->class_mask) {
>  			if (driver_data == ids->driver_data) {

Re: [PATCH] PCI: rework new_id interface for known vendor/device values

[Bandan Das]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Mon, 2014-03-31 at 00:28 -0400, Bandan Das wrote:
>> While using the new_id interface, the user can unintentionally feed
>> incorrect values if the driver static table has a matching entry.
>> This is possible since only the device and vendor fields are
>> mandatory and the rest are optional. As a result, store_new_id
>> will fill in default values that are then passed on to the driver
>> and can have unintended consequences.
>> 
>> As an example, consider the ixgbe driver and the 82599EB network card :
>> echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id
>> 
>> This will pass a driver_data value of 0 to the driver whereas
>> the index 0 in ixgbe actually points to a different set of card
>> operations.
>> 
>> This change automatically selects the matching static entry if there
>> is one for the newly created dynid. However, if the user intentionally
>> wants a different set of values, she must provide all the 7 fields
>> and the static entry will be ignored.
>> 
>> In most cases, this use case seems unnecessary, however, this
>> is a common libvirt/KVM/device assignment scenario where the
>> user might want to bind a device back to the host driver.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 38 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 25f0bc6..187e572 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -90,6 +90,24 @@ static void pci_free_dynids(struct pci_driver *drv)
>>  	spin_unlock(&drv->dynids.lock);
>>  }
>>  
>> +static const struct
>> +pci_device_id *match_id_table_entry(struct device_driver *driver,
>> +				    __u32 vendor, __u32 device)
>> +{
>> +	struct pci_driver *pdrv = to_pci_driver(driver);
>> +	const struct pci_device_id *ids = pdrv->id_table;
>> +
>> +	if (ids) {
>> +		while (ids->vendor || ids->subvendor || ids->class_mask) {
>> +			if ((ids->vendor == vendor) && (ids->device == device))
>> +				return ids;
>> +			ids++;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>  /**
>>   * store_new_id - sysfs frontend to pci_add_dynid()
>>   * @driver: target device driver
>> @@ -102,7 +120,8 @@ static ssize_t
>>  store_new_id(struct device_driver *driver, const char *buf, size_t count)
>>  {
>>  	struct pci_driver *pdrv = to_pci_driver(driver);
>> -	const struct pci_device_id *ids = pdrv->id_table;
>> +	const struct pci_device_id *ids = pdrv->id_table,
>> +		*tids = NULL;
>>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
>>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
>>  	unsigned long driver_data=0;
>> @@ -115,9 +134,24 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
>>  	if (fields < 2)
>>  		return -EINVAL;
>>  
>> -	/* Only accept driver_data values that match an existing id_table
>> -	   entry */
>> -	if (ids) {
>> +	tids = match_id_table_entry(driver, vendor, device);
>> +
>
> Would it make more sense to construct a pci_dev, ex:
>
> if (fields != 7) {
> 	struct pci_dev dev = { .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID };
>
> 	dev.vendor = vendor;
> 	dev.device = device;
> 	if (fields > 2)
> 		dev.subvendor = subvendor;
> 	if (fields > 3)
> 		dev.subdevice = subdevice;
> 	...
>
> 	if (pci_match_id(drv->id_table, &dev))
> 		return -EEXIST;
> }

I initially went ahead this way, but the compilation warns about frame size 
being larger, possibly because of a kernel config option that's set in my config

drivers/pci/pci-driver.c:193:1: warning: the frame size of 
2264 bytes is larger than 2048 bytes [-Wframe-larger-than=]

Do you know if it is safe to ignore this ? This seems to be coming up 
if I add the struct pdev. 

>
>> +	if (tids && (fields != 7)) {
>> +
>> +		subvendor = tids->subvendor;
>> +		subdevice = tids->subdevice;
>> +		class = tids->class;
>> +		class_mask = tids->class_mask;
>> +		driver_data = tids->driver_data;
>
> This doesn't look right.  First, we're potentially overwriting user
> stored data for fields >2 but <7.  Second, we only matched on vendor &
> device and could be filling the rest with data that isn't the best match
> (and is guaranteed to just be a duplicate of a static table ID).
>
>> +
>> +		pr_warn("pci: Using driver (%s) static DeviceID table entry for vendor 0x%04x and device 0x%04x",
>> +			driver->name, vendor, device);
>
> I think we should be error'ing rather than inventing a duplicate ID to
> insert.  How would a user ever know how to use remove_id to clean out
> this new_id?  Thanks,

Ok, makes sense to just error out. Good point about remove_id, 
didn't think about that.

Thanks,
Bandan

> Alex
>
>> +
>> +	} else if (ids) {
>> +
>> +		/* Only accept driver_data values that match an existing
>> +		   id_table entry */
>> +
>>  		retval = -EINVAL;
>>  		while (ids->vendor || ids->subvendor || ids->class_mask) {
>>  			if (driver_data == ids->driver_data) {

Re: [PATCH] PCI: rework new_id interface for known vendor/device values

[Alex Williamson]

On Tue, 2014-04-01 at 14:32 -0400, Bandan Das wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Mon, 2014-03-31 at 00:28 -0400, Bandan Das wrote:
> >> While using the new_id interface, the user can unintentionally feed
> >> incorrect values if the driver static table has a matching entry.
> >> This is possible since only the device and vendor fields are
> >> mandatory and the rest are optional. As a result, store_new_id
> >> will fill in default values that are then passed on to the driver
> >> and can have unintended consequences.
> >> 
> >> As an example, consider the ixgbe driver and the 82599EB network card :
> >> echo "8086 10fb" > /sys/bus/pci/drivers/ixgbe/new_id
> >> 
> >> This will pass a driver_data value of 0 to the driver whereas
> >> the index 0 in ixgbe actually points to a different set of card
> >> operations.
> >> 
> >> This change automatically selects the matching static entry if there
> >> is one for the newly created dynid. However, if the user intentionally
> >> wants a different set of values, she must provide all the 7 fields
> >> and the static entry will be ignored.
> >> 
> >> In most cases, this use case seems unnecessary, however, this
> >> is a common libvirt/KVM/device assignment scenario where the
> >> user might want to bind a device back to the host driver.
> >> 
> >> Signed-off-by: Bandan Das <bsd@redhat.com>
> >> ---
> >>  drivers/pci/pci-driver.c | 42 ++++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 38 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 25f0bc6..187e572 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -90,6 +90,24 @@ static void pci_free_dynids(struct pci_driver *drv)
> >>  	spin_unlock(&drv->dynids.lock);
> >>  }
> >>  
> >> +static const struct
> >> +pci_device_id *match_id_table_entry(struct device_driver *driver,
> >> +				    __u32 vendor, __u32 device)
> >> +{
> >> +	struct pci_driver *pdrv = to_pci_driver(driver);
> >> +	const struct pci_device_id *ids = pdrv->id_table;
> >> +
> >> +	if (ids) {
> >> +		while (ids->vendor || ids->subvendor || ids->class_mask) {
> >> +			if ((ids->vendor == vendor) && (ids->device == device))
> >> +				return ids;
> >> +			ids++;
> >> +		}
> >> +	}
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >>  /**
> >>   * store_new_id - sysfs frontend to pci_add_dynid()
> >>   * @driver: target device driver
> >> @@ -102,7 +120,8 @@ static ssize_t
> >>  store_new_id(struct device_driver *driver, const char *buf, size_t count)
> >>  {
> >>  	struct pci_driver *pdrv = to_pci_driver(driver);
> >> -	const struct pci_device_id *ids = pdrv->id_table;
> >> +	const struct pci_device_id *ids = pdrv->id_table,
> >> +		*tids = NULL;
> >>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
> >>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
> >>  	unsigned long driver_data=0;
> >> @@ -115,9 +134,24 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
> >>  	if (fields < 2)
> >>  		return -EINVAL;
> >>  
> >> -	/* Only accept driver_data values that match an existing id_table
> >> -	   entry */
> >> -	if (ids) {
> >> +	tids = match_id_table_entry(driver, vendor, device);
> >> +
> >
> > Would it make more sense to construct a pci_dev, ex:
> >
> > if (fields != 7) {
> > 	struct pci_dev dev = { .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID };
> >
> > 	dev.vendor = vendor;
> > 	dev.device = device;
> > 	if (fields > 2)
> > 		dev.subvendor = subvendor;
> > 	if (fields > 3)
> > 		dev.subdevice = subdevice;
> > 	...
> >
> > 	if (pci_match_id(drv->id_table, &dev))
> > 		return -EEXIST;
> > }
> 
> I initially went ahead this way, but the compilation warns about frame size 
> being larger, possibly because of a kernel config option that's set in my config
> 
> drivers/pci/pci-driver.c:193:1: warning: the frame size of 
> 2264 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> 
> Do you know if it is safe to ignore this ? This seems to be coming up 
> if I add the struct pdev.

struct pdev is pretty large, maybe just allocate one rather than putting
it on the stack.  The alternative might be the more intrusive path of
passing a struct pci_device_id rather than a struct pci_dev to the
existing match functions.

> >
> >> +	if (tids && (fields != 7)) {
> >> +
> >> +		subvendor = tids->subvendor;
> >> +		subdevice = tids->subdevice;
> >> +		class = tids->class;
> >> +		class_mask = tids->class_mask;
> >> +		driver_data = tids->driver_data;
> >
> > This doesn't look right.  First, we're potentially overwriting user
> > stored data for fields >2 but <7.  Second, we only matched on vendor &
> > device and could be filling the rest with data that isn't the best match
> > (and is guaranteed to just be a duplicate of a static table ID).
> >
> >> +
> >> +		pr_warn("pci: Using driver (%s) static DeviceID table entry for vendor 0x%04x and device 0x%04x",
> >> +			driver->name, vendor, device);
> >
> > I think we should be error'ing rather than inventing a duplicate ID to
> > insert.  How would a user ever know how to use remove_id to clean out
> > this new_id?  Thanks,
> 
> Ok, makes sense to just error out. Good point about remove_id, 
> didn't think about that.

Probably a good idea to check whether libvirt explodes from a write
failure or just ignores it.  Thanks,

Alex

> >> +
> >> +	} else if (ids) {
> >> +
> >> +		/* Only accept driver_data values that match an existing
> >> +		   id_table entry */
> >> +
> >>  		retval = -EINVAL;
> >>  		while (ids->vendor || ids->subvendor || ids->class_mask) {
> >>  			if (driver_data == ids->driver_data) {