aio: ctx->dead cleanups

As Jeff suggested, 1/3 was changed to simply remove the ->dead check. I also updated the changelog.

Jeff, I preserved your acks in 2-3.

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.

And even -EINVAL doesn't look necessary. Yes, if mremap() races with kill_ioctx() vm_munmap(ctx->mmap_base, ctx->mmap_size) can unmap the wrong region. In this case the buggy application should blame itself. And there are other reasons why that vm_munmap() can be wrong. Say, an application can mremap() the part of aio region and then do io_destroy(). We could change aio_ring_remap() to verify vma->that vma_end - vma->vma_start == ctx->mmap_size but this won't help if the application does munmap() instead.

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

---
 fs/aio.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

fs/aio.c +3 -6

index 480440f..893d300 100644
@@ -325,14 +325,11 @@ 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;
+		struct kioctx *ctx = table->table[i];
 
-		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;
-			}
+			ctx->user_id = ctx->mmap_base = vma->vm_start;
+			res = 0;
 			break;
 		}
 	}

Quoted 4 lines · Oleg Nesterov

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 8 lines

And even -EINVAL doesn't look necessary. Yes, if mremap() races with kill_ioctx() vm_munmap(ctx->mmap_base, ctx->mmap_size) can unmap the wrong region. In this case the buggy application should blame itself. And there are other reasons why that vm_munmap() can be wrong. Say, an application can mremap() the part of aio region and then do io_destroy(). We could change aio_ring_remap() to verify vma->that vma_end - vma->vma_start == ctx->mmap_size but this won't help if the application does munmap() instead.

I don't think this paragraph really fits with the patch. It's interesting commentary,

but if you feel strongly enough about it, send a
patch that undoes b2edffdd912b. ;-)

So, drop that paragraph from the commit message, and you can add my reviewed by. The patch itself looks good to me.

Quoted 16 lines · Jeff Moyer

Quoted 4 lines · Oleg Nesterov

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 8 lines

And even -EINVAL doesn't look necessary. Yes, if mremap() races with kill_ioctx() vm_munmap(ctx->mmap_base, ctx->mmap_size) can unmap the wrong region. In this case the buggy application should blame itself. And there are other reasons why that vm_munmap() can be wrong. Say, an application can mremap() the part of aio region and then do io_destroy(). We could change aio_ring_remap() to verify vma->that vma_end - vma->vma_start == ctx->mmap_size but this won't help if the application does munmap() instead.

Quoted 2 lines

I don't think this paragraph really fits with the patch. It's interesting commentary,

Well, this time I disagree. It would be better to add a comment, but the changelog can help too to understand the code and potential problems if it races with kill_ioctx().

Quoted 2 lines

but if you feel strongly enough about it, send a
patch that undoes b2edffdd912b. ;-)

No, I think that commit makes sense. Just it was wrong and we need to fix it. And, in particular, its changelog was wrong (at least confusing), it looks as if there are strong reasons to prevent this race. So the changelog above can unconfuse the git-log reader.

But. I never argue with maintainers about non-technical issues ;)