Re: [Resend PATCH V3 2/4] usb: move struct usb_device->children to struct usb_hub_port->child

[Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>]

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