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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2014-04-01 at 18:00 -0400, Bandan Das wrote:
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Tue, 2014-04-01 at 17:22 -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 returns an error if the user attempts to add a dynid for
> >> a vendor/device combination for which a static entry already exists.
> >> However, if the user intentionally wants a different set of values,
> >> she must provide all the 7 fields and that will be accepted.
> >> 
> >> In KVM/device assignment scenario, the user might want 
> >> to bind a device back to the host driver by writing to new_id
> >> and trip on a possible null pointer dereference.
> >> 
> >> Signed-off-by: Bandan Das <bsd@redhat.com>
> >> ---
> >> v2:
> >> 1. Return error if there is a matching static entry
> >> and change commit message to reflect this behavior
> >> 3. Fill in a pdev and call pci_match_id instead of creating
> >> a new matching function
> >> 4. Change commit message to reflect that libvirt does not
> >> depend on this behavior
> >> 
> >>  drivers/pci/pci-driver.c | 23 ++++++++++++++++++++++-
> >>  1 file changed, 22 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 25f0bc6..493514e 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -103,11 +103,12 @@ 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;
> >> +	struct pci_dev *pdev;
> >>  	__u32 vendor, device, subvendor=PCI_ANY_ID,
> >>  		subdevice=PCI_ANY_ID, class=0, class_mask=0;
> >>  	unsigned long driver_data=0;
> >>  	int fields=0;
> >> -	int retval;
> >> +	int retval = 0;
> >>  
> >>  	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
> >>  			&vendor, &device, &subvendor, &subdevice,
> >> @@ -115,6 +116,26 @@ store_new_id(struct device_driver *driver, const char *buf, size_t count)
> >>  	if (fields < 2)
> >>  		return -EINVAL;
> >>  
> >> +	if (fields != 7) {
> >
> > nit, pdev could be declared within this scope.
> 
> Ok, will spin a new one.
> 
> >> +		pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> >> +		if (!pdev)
> >> +			return -ENOMEM;
> >> +
> >> +		pdev->vendor = vendor;
> >> +		pdev->device = device;
> >> +		pdev->subsystem_vendor = subvendor;
> >> +		pdev->subsystem_device = subdevice;
> >> +		pdev->class = class;
> >
> > Why not class_mask?
> pdev->class_mask isn't used anywhere.

Ah, you're right.  Ok.  Thanks,

Alex

> >> +
> >> +		if (pci_match_id(pdrv->id_table, pdev))
> >> +			retval = -EEXIST;
> >> +
> >> +		kfree(pdev);
> >> +
> >> +		if (retval)
> >> +			return retval;
> >> +	}
> >> +
> >>  	/* Only accept driver_data values that match an existing id_table
> >>  	   entry */
> >>  	if (ids) {