Re: [PATCH] aio_run_iocb should always retry

On Sep 21, 2007, at 3:06 PM, Sarah Sharp wrote:

> When an AIO operation is cancelled, the ki_cancel callback can't
> determine whether the ki_retry callback will ever be called. As a
> result, it can't correctly determine whether to free resources. This
> patch changes aio_run_iocb to always call ki_retry.

Thanks for sending this out.  I spent a few hours staring at the  
cancelation code in fs/aio.c.  I think I have a handle on the sorts  
of things we'll have to address in fs/aio.c to robustly support  
cancellation.

> I've been using gadgetfs (the USB slave filesystem) as an example for
> how to do in-kernel AIO.

Oh dear.  It'd probably be better if you asked linux-aio and we  
updated the docs as you found confusing parts.  Hell, just mail me  
directly.  Anything but having to read that thing and suffer through  
a near-total lack of documenting comments.

>   Both gadgetfs and usbfs2 use callbacks from
> the USB core to know when data is transfered.  If the transfer was a
> write, the completion function simply calls aio_complete() and frees
> data structures.  If it's a read, the completion function function  
> calls
> kick_iocb().  The retry function copies data into userspace, calls
> aio_complete(), and then frees the data structures.
>
> The problem comes when someone calls sys_io_cancel() on a read.  If  
> the
> retry function has started before this point, everything is fine.
> However, if sys_io_cancel() runs before the retry function,  
> aio_run_iocb
> will notice the iocb is cancelled and call aio_complete instead.  The
> retry function never runs, and the data structures are never freed.

So, as written today, fs/aio.c expects ki_cancel to be *very*  
carefully written.  It doesn't serialize ki_cancel calls against,  
well, anything of consequence.  It can be called concurrently.  It  
can race with ki_retry.  It can be called *after* a final ki_retry  
has completed but before the iocb has been removed from the iocb  
list.  (A malicious app can probably get it called the *moment* you  
set ki_cancel, terrifying.)

Along that theme, it's expected to very carefully serialize with  
ki_retry.  It requires that it can serialize with the mechanism that  
ki_retry will rely on to see that aio_complete() or kick_iocb() are  
called.  That requirement, seemingly, requires knowing that being  
called in ki_cancel implies that a previous kick_iocb() call might  
now only drop the iocb ref and won't call ki_retry.

So, to the case you're worried about.  kick_iocb() has been called  
but the bits of epdata haven't been freed yet.  To work with the  
current fs/aio.c code ep_aio_cancel() would have to notice that  
kick_iocb() has been called, but the stuff hasn't been freed, so it  
should free it then.  It might require more locking around that usb  
req queue, who knows.  (commented out spin locks are scary!)

> Jamey and I believe there is no way to know in the cancel function if
> the retry function will be called.

I suspect that you could serialize around whatever queue it is that  
usb_ep_dequeue() is operating on.  I'm not saying that you *should*,  
just that it'd be one way to address the imbalance between what fs/ 
aio.c expects and what ep_aio_cancel() is doing.

> Therefore, kick_iocb() should always call the retry function.  This  
> patch fixes that bug.

In any case, I don't think this is the right solution.  gadgetfs is  
pretty unusual in using one ki_retry to submit and then switching to  
another to copy the result to userspace and free.  The intent of  
ki_retry is to, well, retry.

In particular, if we just fix this freeing problem by calling retry  
after cancellation then we still have the problem where a racing  
ep_aio_cancel() can try and reference freed memory if its called  
after ep_aio_read_retry() returns but before fs/aio.c takes the iocb  
out of the list.

No, I think we should step back and fix the fundamental serialization  
problems with sys_io_cancel().  Right now it requires each ki_cancel  
implementation to take on way too much complexity.  How about we make  
the following rules:

- ki_cancel will not be called while ki_retry is being called

- ki_cancel will not be called after ki_retry returns codes which  
complete the iocb

- ki_cancel will not be called until ki_retry has returned at least once

- ki_cancel after kick_iocb() will not stop ki_retry from being called

- ki_cancel will not be called after aio_complete() has been called

The intention is to only call ki_cancel when we're in that giant idle  
time after when ki_retry has returned either EIOCBQUEUED or  
EIOCBRETRY but before either aio_complete() or kick_iocb() have been  
called.  In the cases where sys_io_cancel() finds that it shouldn't  
call ki_cancel it could return EAGAIN.

Serializing ki_retry and ki_cancel calls will be easy because they're  
called from process context from fs/aio.c.  Serializing with  
aio_complete() will be harder as its called from interrupts from  
subsystems.  We might just add a call to let subsystem aio_complete()  
callers or ki_cancel implementations negotiate who should free  
resources.  Or leave that serialization up to the subsystems.  I'm  
not sure.

If this sounds OK I could roll some patches.  Please tell me that you  
guys have some tests which are failing with the current code so we  
can verify our work.

- z

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel