Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chun Yan Liu writes ("Re: [PATCH V8 3/7] libxl: add pvusb API"):
> On 11/10/2015 at 02:11 AM, in message
> <22080.57829.461049.37192@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
> <Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> > Chunyan Liu writes ("[PATCH V8 3/7] libxl: add pvusb API"): 
> > > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> > > +                               libxl_device_usbctrl *usbctrl, 
> > > +                               libxl__ao_device *aodev) 
> > > +{ 
> >  
> > Thanks for adjusting the error-handling patterns in these functions. 
> > The new way is good, except that: 
> >  
> > > +out: 
> > > +    aodev->rc = rc; 
> > > +    if (rc) aodev->callback(egc, aodev); 
> >  
> > Here, rc is always set, and indeed the code would be wrong if it were 
> > not.  So can you remove the conditional please ?  Ie: 
> 
> Reading the codes, libxl__wait_device_connection will call
> aodev->callback properly.

Indeed.  So you need to call aodev->callback() iff you don't call
libxl__wait_device_connection.  But libxl__wait_device_connection is
called only on the success exit path which ends in `return', not in
`goto out'.  So:

> So here, only if (rc != 0), that means
> error happens, then we need to call aodev->callback to end the
> process. (Refer to current libxl__device_disk_add, all current code
> does similar work.) So I think the code is not wrong (?)

In the `goto out' path, rc is always set.  Writing `if (rc)' implies
that it might not be (or is allowed not to be), which is misleading.


> > > +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, 
> > > +                                  uint8_t *bus, uint8_t *addr) 
> > > +{ 
> > > +    char *filename; 
> > > +    void *buf; 
> > > + 
> > > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid); 
> > > +    if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL)) 
> > > +        *bus = atoi((char *)buf); 
> >  
> > I don't think this cast (and the one for addr) are necessary ? 
> 
> Which cast? Here, we want to get a uint_8 value, but buf is a string,
> we need to do atoi.

I mean that I think

     +        *bus = atoi(buf); 

would compile and be correct.  buf would be automatically converted
from void* to const char*.  It's better to avoid casts where they are
not needed, because casts will suppress compiler warnings.

For example if someone wanted to have the buffer be an updated
parameter they might do this:

  static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid, 
+                                   void **buf,
                                    uint8_t *bus, uint8_t *addr)
-    void *buf; 

and hope or expect the compiler to notice places where they had failed
to update the usage of buf.  With void **buf,
  atoi((char*)buf)
would compile and do a wrong thing whereas
  atoi(buf)
would produce a fatal warning.

> > > +/* bind/unbind usb device interface */ 
> > > +static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath) 
> > > +{ 
...
> > > +        dp = libxl__zalloc(gc, PATH_MAX); 
> > > +        dp = realpath(spath, dp); 
> >  
> > Why is this call to realpath needed ?
> 
> In sysfs, /driver sometimes is a link, since we need to save the original
> driver to xenstore, so need to get the realpath of the driver.

I mean, why is the path with all the symlinks in it not suitable for
storage and later use ?


> > > +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb) 
> > > +{ 
> > ... 
> > > +        /* bind interface to its originial driver */ 
> > > +        drvpath = libxl__xs_read(gc, XBT_NULL, 
> > > +                  GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path", 
> > > +                  usb_encode, usb_interface_xenstore_encode(intf))); 
> > > +        if (drvpath && bind_usb_intf(gc, intf, drvpath)) 
> > > +            LOGE(WARN, "Couldn't bind %s to %s", intf, drvpath); 
> >  
> > This error message could be clearer. 
> >  
> > Also it would be worth a comment in the code to explain why this is a 
> > warning (merely logged), rather than an error (logged and reported 
> > with nonzero status to caller).
> 
> Will update. When doing USB remove, it's the ideal result we can rebound
> the USB to its original driver, in case the USB interface failed to be
> rebound to its original driver, we will export warning but won't stop the
> removing process.

Right.  Please put that in a comment :-).


> > > +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb) 
> > > +{ 
> > ... 
> > > +            if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 
> > > 0) { 
> > > +                LOG(WARN, "Write of %s to node %s failed", drvpath, 
> > > path); 
> > > +            } 
> >  
> > One of the advantages of libxl__xs_write_checked is that it logs 
> > errors.  So you can leave that log message out. 
> >  
> > However, if the xs write fails, you should presumably stop, rather 
> > than carrying on. 
> > 
> > I think you probably do things in the wrong order here.  You should 
> > write the old driver info to xenstore first, so that if the bind to 
> > usbback fails, you have the old driver info. 
> 
> Perhaps not. Once we finished adding entries to xenstore, pvusb
> frontend/backend drivers will detect the change and do probe work, if
> USB device is still not bound to USBBACK, there might be error.

What I mean is this:

Is it not possible to write the original path to xenstore before doing
the unbind ?  Otherwise it seems like there could be error paths where
the original path is not recorded, the xenstore write fails, and then
the information about how to rebind to the original driver has been
lost.

Does writing USBBACK_INFO_PATH"/%s/%s/driver_path" really trigger
usbback ?

> > > +    free(usb_encode); 
> >  
> > This came from the gc, I think. 
...
> > > +    switch (usbctrlinfo.type) { 
> > > +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL: 
> > > +        LOG(ERROR, "Not supported"); 
> > > +        break; 
> >  
> > Needs to set rc. 

(Did you spot these comments ?  Normally I wouldn't even expect you to
quote the comments that you are just going to fix, but since you have
replied to the others, I thought I would check.)


> > > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, 
> > > +                           libxl_device_usb *usb, 
> > > +                           libxl__ao_device *aodev) 
> > > +{ 
> > ... 
> > > +        if (usbctrlinfo.backend_id != 0) { 
> >  
> > LIBXL_TOOLSTACK_DOMID please.  (And in remove, too.) 
> >  
> > Why is this check done in libxl__device_usb_add and do_usb_remove, 
> > rather than in a symmetrical way?
>  
> In libxl__device_usb_add, the set_default function implementation won't
> work if backend domain is USB driver domain in future, so we must add
> a check before set_default, so in libxl__device_usb_add. To check backend
> domain, we need to get usbctrl info. And in do_usb_add, to check usbctrl
> type, again we need to get usbctrl info. It's disgusting to do the same thing
> twice.
> 
> In usb remove, nothing in libxl__device_usb_remove is Dom0 specific, so
> I do all check in do_usb_remove, then only one time to get usbctrl info.

Right.

> If we merge libxl__device_usb_add and do_usb_add, then it's cleaner.

See my comment below.  You've explained the distinction to my
satisfaction.

But, to solve the duplication of the controller info acquisition,
perhaps you could have do_usb_add take the controller info as a
paramaeter ?


> > > +    /* Do the add */ 
> > > +    if (do_usb_add(gc, domid, usb, aodev)) { 
> > > +        rc = ERROR_FAIL; 
> > > +        goto out; 
> > > +    } 
> > > + 
> > > +    libxl__ao_complete(egc, ao, 0); 
> > > +    rc = 0; 
> > > + 
> > > +out: 
> > > +    libxl_device_usbctrl_dispose(&usbctrl); 
> > > +    libxl_usbctrlinfo_dispose(&usbctrlinfo); 
> > > +    aodev->rc = rc; 
> > > +    if (rc) aodev->callback(egc, aodev); 
> > > +    return; 
> >  
> > Calling libxl__ao_complete here is quite wrong.  I think you should 
> > call aodev->callback in both success and failure cases.  And it must 
> > be the last thing you do. 
> 
> As mentioned in libxl__device_usbctrl_add, libxl__ao_complete will
> call callback, so in 'out' section, only if (rc !=0), which means
> error happens, then we need to call aodev->callback explicitly to
> end the process.
> 
> Here, since we don't have actual async work needs to do, so call
> libxl__ao_complete(egc, ao, 0) directly to end the process.

I'm afraid this is definiitely wrong.  There are a number of things
wrong with it, or a number of ways of looking at it:

Most obviously, a single entrypoint should always complete its
execution the same way.  In this case the entrypoint takes an aodev
and in the error path calls aodev->callback().  So it should do the
same on the success path.

Secondly (and relatedly), it is generally wrong for anything inside a
particular piece of machinery to terminate an ao.

In this particular case I think it leads to a use-after-free bug:
completing the ao will (maybe) free the ao, and aodev was allocated
from the ao's gc.  So the write to aodev->rc is improper.


> > I might ask the same question about do_usb_add and 
> > libxl__device_usb_add ? 
> 
> Using libxl__device_usb_add and within it extracting do_usb_add is to:
> * doing some initialization work and necessary check work in
>   libxl__device_usb_add (this part are common to PVUSB and QEMU USB)
> * doing actual adding usb operations in do_usb_add (this part will diverge
>   for PVUSB and QEMU USB) .

This is a good reason for keeping them split.  Thanks for the
explanation.


Thanks for your other replies.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel