aio: ctx->dead cleanups

Al, please help. We are trying to backport some aio fixes and I am absolutely confused by your b2edffdd912b "fix mremap() vs. ioctx_kill() race".

Firstly, I simply can't understand what exactly it tries to fix. OK, aio_free_ring() can race with kill and we can remap the soon-to-be-killed ctx. So what? kill_ioctx() will the the correct (already re-mapped) ctx->mmap_base after it drops mm->ioctx_lock.

Huh? kill_ioctx() picks ctx->mmap_base and passes it to vm_munmap(). Which tries to grab mmap_sem, blocks for mremap() from another thread and waits for it to drop mmap_sem. By that time ctx->mmap_base has nothing whatsoever to the argument we'd passed to vm_munmap(). Sure, it had been recalculated by aio_ring_remap(), but it's too late for us - we'd already fetched the old value.

Quoted 9 lines · Oleg Nesterov

Al, please help. We are trying to backport some aio fixes and I am absolutely confused by your b2edffdd912b "fix mremap() vs. ioctx_kill() race".

Firstly, I simply can't understand what exactly it tries to fix. OK, aio_free_ring() can race with kill and we can remap the soon-to-be-killed ctx. So what? kill_ioctx() will the the correct (already re-mapped) ctx->mmap_base after it drops mm->ioctx_lock.

Huh? kill_ioctx() picks ctx->mmap_base and passes it to vm_munmap(). Which tries to grab mmap_sem, blocks for mremap() from another thread and waits for it to drop mmap_sem. By that time ctx->mmap_base has nothing whatsoever to the argument we'd passed to vm_munmap().

Quoted 12 lines · Al Viro

Quoted 9 lines · Oleg Nesterov

Al, please help. We are trying to backport some aio fixes and I am absolutely confused by your b2edffdd912b "fix mremap() vs. ioctx_kill() race".

Firstly, I simply can't understand what exactly it tries to fix. OK, aio_free_ring() can race with kill and we can remap the soon-to-be-killed ctx. So what? kill_ioctx() will the the correct (already re-mapped) ctx->mmap_base after it drops mm->ioctx_lock.

Quoted 4 lines

Huh? kill_ioctx() picks ctx->mmap_base and passes it to vm_munmap(). Which tries to grab mmap_sem, blocks for mremap() from another thread and waits for it to drop mmap_sem. By that time ctx->mmap_base has nothing whatsoever to the argument we'd passed to vm_munmap().

Yes. But it seems that you missed another part of my email:

	So it seems to me we only need this change to ensure that move_vma() can
	not succeed if ctx was already removed from ->ioctx_table, or, if we race
	with ioctx_alloc(), it was not added to ->ioctx_table. IOW, we need to
	ensure that move_vma()->aio_ring_mmap() can not race with
	vm_munmap(ctx->mmap_base) in kill_ioctx() or ioctx_alloc(). And this race
	doesn't look really bad. The kernel can't crash, just the application can
	fool itself.

So once again, could explain why do we really need to prevent this? Afaics, if the application is stupid, it can only fool itself.

And please note that ctx->mmap_base or/and ctx->mmap_size can be wrong anyway. Say, an application can munmap() this vma, or munmap() the part of this vma.

Quoted 6 lines · Al Viro

Huh? kill_ioctx() picks ctx->mmap_base and passes it to vm_munmap(). Which tries to grab mmap_sem, blocks for mremap() from another thread and waits for it to drop mmap_sem. By that time ctx->mmap_base has nothing whatsoever to the argument we'd passed to vm_munmap(). Sure, it had been recalculated by aio_ring_remap(), but it's too late for us - we'd already fetched the old value.

And yes, the leak you've spotted is real, but I would very much prefer to avoid that goto - something like this instead:

mm/mremap.c +0 -0

index 034e2d3..b36b530 100644

Quoted 29 lines · Oleg Nesterov

Quoted 12 lines · Al Viro

Quoted 9 lines · Oleg Nesterov

Al, please help. We are trying to backport some aio fixes and I am absolutely confused by your b2edffdd912b "fix mremap() vs. ioctx_kill() race".

Firstly, I simply can't understand what exactly it tries to fix. OK, aio_free_ring() can race with kill and we can remap the soon-to-be-killed ctx. So what? kill_ioctx() will the the correct (already re-mapped) ctx->mmap_base after it drops mm->ioctx_lock.

Quoted 4 lines

Huh? kill_ioctx() picks ctx->mmap_base and passes it to vm_munmap(). Which tries to grab mmap_sem, blocks for mremap() from another thread and waits for it to drop mmap_sem. By that time ctx->mmap_base has nothing whatsoever to the argument we'd passed to vm_munmap().

Yes. But it seems that you missed another part of my email:

	So it seems to me we only need this change to ensure that move_vma() can
	not succeed if ctx was already removed from ->ioctx_table, or, if we race
	with ioctx_alloc(), it was not added to ->ioctx_table. IOW, we need to
	ensure that move_vma()->aio_ring_mmap() can not race with
	vm_munmap(ctx->mmap_base) in kill_ioctx() or ioctx_alloc(). And this race
	doesn't look really bad. The kernel can't crash, just the application can
	fool itself.

Quoted 6 lines

So once again, could explain why do we really need to prevent this? Afaics, if the application is stupid, it can only fool itself.

And please note that ctx->mmap_base or/and ctx->mmap_size can be wrong anyway. Say, an application can munmap() this vma, or munmap() the part of this vma.

And speaking of aio_ring_remap() it can "corrupt" ->mmap_base even with this patch. Just you need to mremap() the tail of aio-mapped memory.

No?

Oleg.

atomic_read(&ctx->dead) in aio_ring_remap() looks confusing. kill_ioctx() sets ctx->dead and removes ctx from ->ioctx_table "atomically" under mm->ioctx_lock, so aio_ring_remap() can never see a dead ctx.

If we really want this check, we should put it under WARN_ON() and it should not depend on aio_ring_file == file.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

fs/aio.c +8 -8

index 480440f..0693333 100644
@@ -325,14 +325,14 @@ static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
 	rcu_read_lock();
 	table = rcu_dereference(mm->ioctx_table);
 	for (i = 0; i < table->nr; i++) {
-		struct kioctx *ctx;
-
-		ctx = table->table[i];
-		if (ctx && ctx->aio_ring_file == file) {
-			if (!atomic_read(&ctx->dead)) {
-				ctx->user_id = ctx->mmap_base = vma->vm_start;
-				res = 0;
-			}
+		struct kioctx *ctx = table->table[i];
+
+		if (!ctx || WARN_ON(atomic_read(&ctx->dead)))
+			continue;
+
+		if (ctx->aio_ring_file == file) {
+			ctx->user_id = ctx->mmap_base = vma->vm_start;
+			res = 0;
 			break;
 		}
 	}

aio_free_ring() can actually see the NULL page in ->ring_pages[], this can happen if aio_setup_ring() fails.

And in this case page_count(ctx->ring_pages[i]) can OOPS.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

fs/aio.c +4 -4

index b605ab2..666fbb8 100644
@@ -292,12 +292,12 @@ static void aio_free_ring(struct kioctx *ctx)
 	put_aio_ring_file(ctx);
 
 	for (i = 0; i < ctx->nr_pages; i++) {
-		struct page *page;
-		pr_debug("pid(%d) [%d] page->count=%d\n", current->pid, i,
-				page_count(ctx->ring_pages[i]));
-		page = ctx->ring_pages[i];
+		struct page *page = ctx->ring_pages[i];
 		if (!page)
 			continue;
+
+		pr_debug("pid(%d) [%d] page->count=%d\n",
+				current->pid, i, page_count(page));
 		ctx->ring_pages[i] = NULL;
 		put_page(page);
 	}

Quoted 5 lines · Oleg Nesterov

atomic_read(&ctx->dead) in aio_ring_remap() looks confusing. kill_ioctx() sets ctx->dead and removes ctx from ->ioctx_table "atomically" under mm->ioctx_lock, so aio_ring_remap() can never see a dead ctx.

Quoted 2 lines

If we really want this check, we should put it under WARN_ON() and it should not depend on aio_ring_file == file.

Just get rid of the check, it doesn't make any sense.

-Jeff

Quoted 33 lines

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

fs/aio.c +8 -8

index 480440f..0693333 100644
@@ -325,14 +325,14 @@ static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
 	rcu_read_lock();
 	table = rcu_dereference(mm->ioctx_table);
 	for (i = 0; i < table->nr; i++) {
-		struct kioctx *ctx;
-
-		ctx = table->table[i];
-		if (ctx && ctx->aio_ring_file == file) {
-			if (!atomic_read(&ctx->dead)) {
-				ctx->user_id = ctx->mmap_base = vma->vm_start;
-				res = 0;
-			}
+		struct kioctx *ctx = table->table[i];
+
+		if (!ctx || WARN_ON(atomic_read(&ctx->dead)))
+			continue;
+
+		if (ctx->aio_ring_file == file) {
+			ctx->user_id = ctx->mmap_base = vma->vm_start;
+			res = 0;
 			break;
 		}
 	}

fs/aio.c static int aio_ring_remap(struct file *file, struct vm_area_struct *vma) · line 330

Quoted 7 lines · Oleg Nesterov

fs/aio.c +1 -1

@@ -327,7 +327,7 @@ static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
 	for (i = 0; i < table->nr; i++) {
 		struct kioctx *ctx = table->table[i];
 
-		if (!ctx || WARN_ON(atomic_read(&ctx->dead)))
+		if (!ctx || ctx->dead)

Argh, sorry, I removed WARN_ON() by accident.

------------------------------------------------------------------------------
Subject: [PATCH v2 2/3] aio: make aio_ring->dead boolean

"atomic_t dead" makes no sense. atomic_read() is the plain LOAD, it doesn't have some "additional" synchronization with xchg().

And now that kill_ioctx() sets "dead" under mm->ioctx_lock we do not even need xchg().

Quoted 11 lines · Al Viro

On Wed, Jun 17, 2015 at 01:39:06AM +0100, Al Viro wrote:

And yes, the leak you've spotted is real, but I would very much prefer to avoid that goto - something like this instead:

mm/mremap.c +0 -0

index 034e2d3..b36b530 100644

mm/mremap.c static unsigned long move_vma(struct vm_area_struct *vma, · line 297

Quoted 8 lines

mm/mremap.c +4 -1

@@ -291,7 +291,10 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 		if (err < 0) {
 			move_page_tables(new_vma, new_addr, vma, old_addr,
 					 moved_len, true);
-			return err;
+			vma = new_vma;
+			old_len = new_len;
+			old_addr = new_addr;
+			new_addr = err;

Personally, I'd really prefer to factor out at least this move_page_tables() with six args. Although I agree, "goto previous_if" doesn't look nice too, this needs cleanup.

But this is minor. I am already sleeping, most probably I misread this code. But it seems that there is another bug with VM_ACCOUNT.

I'll recheck tomorrow and write another email.