On Thu, 14 Jun 2012, Lan Tianyu wrote:
> >> --- a/drivers/usb/core/devices.c
> >> +++ b/drivers/usb/core/devices.c
> >
> >> @@ -589,20 +590,20 @@ static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes,
> >> free_pages((unsigned long)pages_start, 1);
> >>
> >> /* Now look at all of this device's children. */
> >> - for (chix = 0; chix< usbdev->maxchild; chix++) {
> >> - struct usb_device *childdev = usbdev->children[chix];
> >> -
> >> + usb_get_hub_each_child(usbdev, chix, childdev)
> >> + pr_info("%s: %d\n", __func__, chix);
> >> if (childdev) {
> >> usb_lock_device(childdev);
> >> ret = usb_device_dump(buffer, nbytes, skip_bytes,
> >> file_offset, childdev, bus,
> >> - level + 1, chix, ++cnt);
> >> + level + 1, chix - 1, ++cnt);
> >> usb_unlock_device(childdev);
> >> + usb_put_dev(childdev);
> >> if (ret == -EFAULT)
> >> return total_written;
> >> total_written += ret;
> >> }
> >> - }
> >> +
> >
> > This is wrong. Without any {}'s, only the pr_info() statement is
> > inside the loop.
> >
> > Why did you add the pr_info() statement, anyway?
> Oh. Sorry. This is debug code which should be removed.
> It should be
> usb_get_hub_each_child(usbdev, chix, childdev)
> if (childdev) {
> usb_lock_device(childdev);
> ret = usb_device_dump(buffer, nbytes, skip_bytes,
> file_offset, childdev, bus,
> level + 1, chix - 1, ++cnt);
> }
Except that it really should be:
usb_get_hub_each_child(usbdev, chix, childdev) {
if (childdev) {
...
}
}
with an outer set of {}s. Otherwise somebody will make the same
mistake you did when they insert a debugging statement in the middle.
> >> +/**
> >> + * usb_get_hub_child - Get the pointer of child device
> >> + * attached to the port which is specified by @port1.
> >> + * @hdev: USB device belonging to the usb hub
> >> + * @port1: port num to indicate which port the child device
> >> + * is attached to.
> >> + *
> >> + * USB drivers call this function to get hub's child device
> >> + * pointer. The child device's refcount will be increased
> >> + * before return. The caller should invoke usb_put_dev() to
> >> + * decrease the device's refcount when the child will not be
> >> + * used.
> >
> > Why bother to do this? If there's a race, and the child could be
> > removed after this function returns, then in fact the child could be
> > removed even while this function is running. usb_get_dev() won't
> > protect against that.
> >
> This is suggested by Greg. Refcount increase can prevent child's struct
> usb_device from being free. At last this can keep child point vaild.
I'm sorry, but Greg was wrong. See below.
> >> +struct usb_device *usb_get_hub_child(struct usb_device *hdev,
> >> + int port1)
> >> +{
> >> + struct usb_hub *hub = hdev_to_hub(hdev);
> >> +
> >> + if (port1< 1 || port1> hdev->maxchild)
> >> + return NULL;
> >> + return usb_get_dev(hub->port_data[port1 - 1].child);
> >
> > What is the reason for calling usb_get_dev() here? If the old code
> > didn't need it, why does the new code?
>
> Just for more safe, the function return the struct usb_device and the point
> may be invalid caused by device being released(memory being freed). But caller
> may still use it. This will cause oops. Last time, we discussed this may happen
> in the r8a66597-hcd.c and cc the maintainer but have not got response.
What happens if the child device is released after your code evaluates
hub->port_data[port1 - 1].child
but before usb_get_dev() is called? There will still be an oops.
No, the caller of usb_get_hub_child() has to be entirely responsible
for making certain that children don't get released unexpectedly.
Calling usb_get_dev() like this won't help.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
