Re: [PATCH 3/5] fs: remove ki_nbytes

[Al Viro <viro@ZenIV.linux.org.uk>]

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, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>