libxl_pci: verify device is assignable, rename misleading function (v2)

PATCH New Doug Magee 2 participants 5 messages Jan 17, 2012 – Feb 20, 2012

Share & tools

Doug Magee

author

2012-01-17 17:26 UTC #0 lore raw

PATCH 1/1 libxl_pci: verify device is assignable, rename misleading function (v2)

This series contains one patch that changes the behavior of libxl__device_pci_add to verify the device is properly assignable. There's also a patch that renames a function so it's not misleading.

This second version explicitly frees the values returned by libxl_device_pci_list_assignable.

Doug Magee

author

2012-01-17 17:26 UTC #1 lore raw

PATCH 1/1 libxl_pci: rename is_assigned to is_pcidev_in_array

All this function does is check to see if a device is in an array of pcidevs passed by the caller. The function name can be misleading if ever used to check against a list of devices other than those assigned to a domain.

Signed-off-by Doug Magee <[email protected]>

tools/libxl/libxl_pci.c +4 -4

diff -r 5b2676ac1321 -r eb59afed10a6 tools/libxl/libxl_pci.c
@@ -461,7 +461,7 @@ static int get_all_assigned_devices(libx
     return 0;
 }
 
-static int is_assigned(libxl_device_pci *assigned, int num_assigned,
+static int is_pcidev_in_array(libxl_device_pci *assigned, int num_assigned,
                        int dom, int bus, int dev, int func)
 {
     int i;

@@ -510,7 +510,7 @@ libxl_device_pci *libxl_device_pci_list_
         if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
             continue;
 
-        if ( is_assigned(assigned, num_assigned, dom, bus, dev, func) )
+        if ( is_pcidev_in_array(assigned, num_assigned, dom, bus, dev, func) )
             continue;
 
         new = realloc(pcidevs, ((*num) + 1) * sizeof(*new));

@@ -802,7 +802,7 @@ int libxl__device_pci_add(libxl__gc *gc,
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot determine if device is assigned, refusing to continue");
         goto out;
     }
-    if ( is_assigned(assigned, num_assigned, pcidev->domain,
+    if ( is_pcidev_in_array(assigned, num_assigned, pcidev->domain,
                      pcidev->bus, pcidev->dev, pcidev->func) ) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device already attached to a domain");
         rc = ERROR_FAIL;

@@ -906,7 +906,7 @@ static int do_pci_remove(libxl__gc *gc, 
         return ERROR_FAIL;
 
     rc = ERROR_INVAL;
-    if ( !is_assigned(assigned, num, pcidev->domain,
+    if ( !is_pcidev_in_array(assigned, num, pcidev->domain,
                       pcidev->bus, pcidev->dev, pcidev->func) ) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device not attached to this domain");
         goto out_fail;

Doug Magee

author

2012-01-17 17:26 UTC #2 lore raw

PATCH 1/1 libxl_pci: verify device is assignable before adding to domain

In reply to Doug Magee

Previously, libxl__device_pci_add only checked the device to be added against the list of currently assigned devices. This patch changes the behavior to check that the device to be assigned is in the list of assignable devices, which only includes those owned by pciback and not assigned to another domain.

Signed-off-by Doug Magee <[email protected]>

tools/libxl/libxl_pci.c +9 -9

diff -r eb59afed10a6 -r 6a3f270992eb tools/libxl/libxl_pci.c
@@ -793,18 +793,15 @@ int libxl__device_pci_add(libxl__gc *gc,
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int orig_vdev, pfunc_mask;
-    libxl_device_pci *assigned;
-    int num_assigned, i, rc;
+    libxl_device_pci *assignable;
+    int num_assignable, i, rc;
     int stubdomid = 0;
 
-    rc = get_all_assigned_devices(gc, &assigned, &num_assigned);
-    if ( rc ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "cannot determine if device is assigned, refusing to continue");
-        goto out;
-    }
-    if ( is_pcidev_in_array(assigned, num_assigned, pcidev->domain,
+    assignable = libxl_device_pci_list_assignable(ctx, &num_assignable);
+
+    if ( !is_pcidev_in_array(assignable, num_assignable, pcidev->domain,
                      pcidev->bus, pcidev->dev, pcidev->func) ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device already attached to a domain");
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device is either assigned to another domain, or not owned by pciback");
         rc = ERROR_FAIL;
         goto out;
     }

@@ -856,6 +853,9 @@ int libxl__device_pci_add(libxl__gc *gc,
     }
 
 out:
+    for ( i = 0; i < num_assignable; i++ )
+        libxl_device_pci_dispose(&assignable[i]);
+    free(assignable);
     return rc;
 }

Ian Jackson

2012-01-24 16:06 UTC #3 lore raw

Re: [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain

Doug Magee writes ("[PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain"):

Quoted 4 lines

Previously, libxl__device_pci_add only checked the device to be added against the list of currently assigned devices. This patch changes the behavior to check that the device to be assigned is in the list of assignable devices, which only includes those owned by pciback and not assigned to another domain.

...

Quoted 2 lines

tools/libxl/libxl_pci.c +2 -0

+    libxl_device_pci *assignable;
+    int num_assignable, i, rc;

...

tools/libxl/libxl_pci.c int libxl__device_pci_add(libxl__gc *gc, · line 801

Quoted 2 lines

tools/libxl/libxl_pci.c +2 -0

+    assignable = libxl_device_pci_list_assignable(ctx, &num_assignable);
+

What about failure ? libxl_device_pci_list_assignable might return NULL on failure, I think. So you need to add a check for that.

And in that case it won't assign to num_assignable either, leading to an uninitialised variable use and possible crash in out. I think you should initialise assignable and num_assignable to 0 to avoid this.

tools/libxl/libxl_pci.c int libxl__device_pci_add(libxl__gc *gc, · line 804

Quoted 2 lines

tools/libxl/libxl_pci.c +1 -1

-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device already attached to a domain");
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "PCI device is either assigned to another domain, or not owned by pciback");

While you're here, would you please wrap this line to within 75-80 columns ?

Thanks,
Ian.

Ian Jackson

2012-02-20 16:28 UTC #4 lore raw

Re: [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain

Ian Jackson writes ("Re: [Xen-devel] [PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain"):

Quoted 6 lines

Doug Magee writes ("[PATCH 2 of 2] libxl_pci: verify device is assignable before adding to domain"):
Quoted 4 lines
Previously, libxl__device_pci_add only checked the device to be added against the list of currently assigned devices. This patch changes the behavior to check that the device to be assigned is in the list of assignable devices, which only includes those owned by pciback and not assigned to another domain.
...

Quoted 8 lines

Quoted 2 lines
tools/libxl/libxl_pci.c +2 -0
+    libxl_device_pci *assignable;
+    int num_assignable, i, rc;
...
tools/libxl/libxl_pci.c int libxl__device_pci_add(libxl__gc *gc, · line 801
Quoted 2 lines
tools/libxl/libxl_pci.c +2 -0
+    assignable = libxl_device_pci_list_assignable(ctx, &num_assignable);
+
What about failure ? libxl_device_pci_list_assignable might return NULL on failure, I think. So you need to add a check for that.

Yes.

Quoted 3 lines

And in that case it won't assign to num_assignable either, leading to an uninitialised variable use and possible crash in out. I think you should initialise assignable and num_assignable to 0 to avoid this.

Yes.

Thanks,
Ian.