Re: [PATCH v4 1/3] aio_ring_remap: kill the bogus ctx->dead check

[Oleg Nesterov <oleg@redhat.com>]

On 06/18, Jeff Moyer wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > 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.
>
> 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().

> 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 ;)

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

OK, I am sending v5 with your reviewed-by.

Thanks!

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/