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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150619175506.GA19348@redhat.com>
Date:	Fri, 19 Jun 2015 19:55:06 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jeff Moyer <jmoyer@...hat.com>
Cc:	Al Viro <viro@...IV.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benjamin LaHaise <bcrl@...ck.org>, linux-aio@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/3] aio_ring_remap: kill the bogus ctx->dead check

On 06/18, Jeff Moyer wrote:
>
> Oleg Nesterov <oleg@...hat.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/