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 <[email protected]>
> ---
> 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) {
Alex Williamson <[email protected]> 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 <[email protected]>
>> ---
>> 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) {
On Tue, 2014-04-01 at 14:32 -0400, Bandan Das wrote:
> Alex Williamson <[email protected]> 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 <[email protected]>
> >> ---
> >> 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) {