On Mon, 15 Jan 2007, David Brownell wrote:
> On Friday 12 January 2007 2:12 pm, Alan Stern wrote:
>
> > On the other hand, I did find (another) bug in the async I/O handling in
> > inode.c. If you aren't using the AIO code in usb.c then it shouldn't
> > affect you, but you might want to give it a try anyway.
>
> Alan, could you explain what the bug here is? ISTR that the policy
> for those retry routines is to always put the request. I know that
> you removed that some time ago, and that it re-appeared with a recent
> update to change the AIO interface ... but the original code had no
> leaks, and this change seems likely to create one.
On the contrary, this isn't a leak (i.e., permanent loss of memory caused
by failure to release a reference). Just the opposite -- it is memory
corruption caused by decrementing a refcount inappropriately and thus
allowing a data structure to be freed while it is still in use.
I really have no idea what has changed and what hasn't. Looking through
some old kernel trees, it's apparent that the ep_aio_read_retry() did
change between 2.6.18 and 2.6.19, reintroducing the aio_put_req() call and
thereby reverting the earlier bugfix. That's a pity; I don't enjoy
finding & fixing refcount bugs, and I enjoy even less having to fix the
same bug twice.
> (Unless the AIO
> core changed incompatibly ... but I'd have expected that the person
> who updated this code to reflect the "new" API would get that right.)
I also can't explain the AIO policy/interface/API -- because as far as I
can tell, it is completely undocumented! There's nothing to go by except
the source code for the AIO core. Here are the relevant extracts from
fs/aio.c:
static int __aio_run_iocbs(struct kioctx *ctx)
{
...
while (!list_empty(&run_list)) {
iocb = list_entry(run_list.next, struct kiocb,
ki_run_list);
list_del(&iocb->ki_run_list);
/*
* Hold an extra reference while retrying i/o.
*/
iocb->ki_users++; /* grab extra reference */
aio_run_iocb(iocb);
if (__aio_put_req(ctx, iocb)) /* drop extra ref */
put_ioctx(ctx);
}
...
}
So the core acquires and releases its own reference around the call of the
ki_wait method.
static ssize_t aio_run_iocb(struct kiocb *iocb)
{
...
if (!(retry = iocb->ki_retry)) {
printk("aio_run_iocb: iocb->ki_retry = NULL\n");
return 0;
}
...
BUG_ON(current->io_wait != NULL);
current->io_wait = &iocb->ki_wait;
ret = retry(iocb);
current->io_wait = NULL;
if (ret != -EIOCBRETRY && ret != -EIOCBQUEUED) {
BUG_ON(!list_empty(&iocb->ki_wait.task_list));
aio_complete(iocb, ret, 0);
}
...
}
So the core does not expect the ki_retry method to change iocb's refcount.
Alan Stern
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
