Re: GPF in aio_migratepage

[Gu Zheng]

Hi Dave,
According to your analysis and the dump stack, it seems that it tried to migrate
aio ring pages in the compact path, but the aio context(mapping->private_data) is
invalid (not NULL), so only one case can cause this condition, aio ring file was
left and not cleaned up in the fail path of ioctx_alloc.

So please try the following patch, I think it can fix this issue.


Signed-off-by: Gu Zheng <guz.fnst@xxxxxxxxxxxxxx>
---
 fs/aio.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 08159ed..5255548 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -367,8 +367,10 @@ static int aio_setup_ring(struct kioctx *ctx)
 	if (nr_pages > AIO_RING_PAGES) {
 		ctx->ring_pages = kcalloc(nr_pages, sizeof(struct page *),
 					  GFP_KERNEL);
-		if (!ctx->ring_pages)
+		if (!ctx->ring_pages) {
+			put_aio_ring_file(ctx);
 			return -ENOMEM;
+		}
 	}
 
 	ctx->mmap_size = nr_pages * PAGE_SIZE;
-- 
1.7.7



On 11/26/2013 11:56 PM, Dave Jones wrote:

> On Tue, Nov 26, 2013 at 10:23:37AM -0500, Benjamin LaHaise wrote:
>  > On Mon, Nov 25, 2013 at 11:19:53PM -0800, Kent Overstreet wrote:
>  > > On Tue, Nov 26, 2013 at 01:01:32AM -0500, Dave Jones wrote:
>  > > > On Mon, Nov 25, 2013 at 10:26:45PM -0500, Dave Jones wrote:
>  > > >  > Hi Kent,
>  > > >  > 
>  > > >  > I hit the GPF below on a tree based on 8e45099e029bb6b369b27d8d4920db8caff5ecce
>  > > >  > which has your commit e34ecee2ae791df674dfb466ce40692ca6218e43
>  > > >  > ("aio: Fix a trinity splat").  Is this another path your patch missed, or
>  > > >  > a completely different bug to what you were chasing ?
>  > > > 
>  > > > And here's another from a different path, this time on 32bit.
>  > 
>  > For Dave: what line is this bug on?  Is it the dereference of ctx when 
>  > doing spin_lock_irqsave(&ctx->completion_lock, flags); or is the 
>  > ctx->ring_pages[idx] = new; ?
> 
>>From the 32bit trace:
> 
>> EIP is at aio_migratepage+0xad/0x126
> 
> disasm of aio.o shows aio_migratepage at 0x6f5, which means we oopsed at 7a2...
> 
> 
> 
>                         ctx->ring_pages[idx] = new;
>      79f:       8b 57 50                mov    0x50(%edi),%edx
>      7a2:       89 34 82                mov    %esi,(%edx,%eax,4)
>         raw_spin_unlock_irq(&lock->rlock);
> 
> which matches up with the Code: line.
> 
> So that's actually..
> 
> 	spin_unlock_irqrestore(&ctx->completion_lock, flags);
> 
> 
> The 64bit trace looks a little funky due to gcc optimising and moving
> things around, but I think it's the same thing except this time it's
> in the lock acquire path instead of lock release.
> 
>> aio_migratepage+0xa6/0x150
> 
> aio_migratepage is at 0x540, and at 0x5e6, we see...
> 
>          */
>         spin_lock(&mapping->private_lock);
>         ctx = mapping->private_data;
>      5c3:       4d 8b ad a8 01 00 00    mov    0x1a8(%r13),%r13
>         if (ctx) {
>      5ca:       4d 85 ed                test   %r13,%r13
>      5cd:       0f 84 85 00 00 00       je     658 <aio_migratepage+0x118>
>                 pgoff_t idx;
>                 spin_lock_irqsave(&ctx->completion_lock, flags);
>      5d3:       49 8d 95 c8 02 00 00    lea    0x2c8(%r13),%rdx
>      5da:       48 89 d7                mov    %rdx,%rdi
>      5dd:       48 89 55 c8             mov    %rdx,-0x38(%rbp)
>      5e1:       e8 00 00 00 00          callq  5e6 <aio_migratepage+0xa6>
>                 migrate_page_copy(new, old);
>      5e6:       48 89 de                mov    %rbx,%rsi
>      5e9:       4c 89 e7                mov    %r12,%rdi
>          */
>         spin_lock(&mapping->private_lock);
>         ctx = mapping->private_data;
>         if (ctx) {
>                 pgoff_t idx;
>                 spin_lock_irqsave(&ctx->completion_lock, flags);
> 
> 
>  > Actually, is there easy way to reproduce this with Trinity?  I can have a 
>  > look if you point me in the right direction.
> 
> I've not found a simple reproducer recipe yet, working on it.
> So far I've just been running it for an hour and waiting. If I can narrow down
> the syscalls necessary I'll let you know.
>  
> 	Dave
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/