On 02/06/2015 08:03 AM, Al Viro wrote:
> On Thu, Feb 05, 2015 at 10:29:42AM -0500, Alan Stern wrote:
>> On Wed, 4 Feb 2015, Al Viro wrote:
>>
>>>>> Um... readv() is also going through ->aio_read().
>>>>
>>>> Why does readv() do this but not read()? Wouldn't it make more sense
>>>> to have all the read* calls use the same internal interface?
>>>
>>> Because there are two partially overlapping classes wrt vector IO semantics:
>> ...
>>
>> Thanks for the detailed explanation. It appears to boil down to a
>> series of historical accidents.
>>
>> In any case, feel free to copy the non-isochronous behavior of the
>> synchronous routines in the async routines. It certainly won't hurt
>> anything.
>
> Hmm... What happens if f_fs.c successfully queues struct usb_request, returns
> -EIOCBQUEUED and then gets hit by io_cancel(2)? AFAICS, you get
> ffs_aio_cancel() called, which dequeues usb_request and buggers off.
> The thing is, freeing io_data and stuff hanging off it would be done by
> ffs_user_copy_worker(), which would be scheduled via schedule_work() by
> ffs_epfile_async_io_complete(), i.e. usb_request ->complete() callback.
> And usb_ep_dequeue() (aka. ep->ops->dequeue()) has tons of instances, but
> AFAICS some of them might not trigger usb_gadget_giveback_request(), which
> would normally call ->complete()...
>
> Example:
> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct net2272_ep *ep;
> struct net2272_request *req;
> unsigned long flags;
> int stopped;
>
> ep = container_of(_ep, struct net2272_ep, ep);
> if (!_ep || (!ep->desc && ep->num != 0) || !_req)
> return -EINVAL;
>
> spin_lock_irqsave(&ep->dev->lock, flags);
> stopped = ep->stopped;
> ep->stopped = 1;
>
> /* make sure it's still queued on this endpoint */
> list_for_each_entry(req, &ep->queue, queue) {
> if (&req->req == _req)
> break;
> }
> if (&req->req != _req) {
> spin_unlock_irqrestore(&ep->dev->lock, flags);
> return -EINVAL;
> }
>
> /* queue head may be partially complete */
> if (ep->queue.next == &req->queue) {
> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
> net2272_done(ep, req, -ECONNRESET);
> }
> req = NULL;
> ep->stopped = stopped;
>
> spin_unlock_irqrestore(&ep->dev->lock, flags);
> return 0;
> }
>
> Note that net2272_done(), which would call usb_gadget_giveback_request(),
> is only called if the victim happens to be queue head. Is that just a
> net2272.c bug, or am I missing something subtle here? Looks like
> at least on that hardware io_cancel() could leak io_data and everything
> that hangs off it...
>
> FWIW, net2272.c was the first one I looked at (happened to be on the last
> line of screen during git grep for \.dequeue in drivers/usb/gadget ;-)
> and after checking several more it seems that it's a Sod's Law in action -
> I'd checked about 5 of them and they seem to call usb_gadget_giveback_request()
> as long as they find the sucker in queue, head or no head. OTOH, there's
> a lot more of those guys, so that observation is not worth much...
>
> IOW, is that a net2272.c bug, or a f_fs.c one?
AFAIK usb request should be completed in all cases, and many gadget
drivers make assumptions that complete() of each request will be called,
so it's definitely bug in net2272 driver.
Robert Baldyga
--
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
