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?
--
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
