linux-kernel - Re: git pull -- [PATCH] aio: v2 ensure access to ctx->ring

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: <CA+55aFx7vg2rvOu6Bu_e8+BB=ymoUMp0AM9JmAuUuSgo0LVEwg@mail.gmail.com>
Date:	Thu, 27 Mar 2014 12:43:13 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Benjamin LaHaise <bcrl@...ck.org>
Cc:	Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Tang Chen <tangchen@...fujitsu.com>,
	Gu Zheng <guz.fnst@...fujitsu.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	stable <stable@...r.kernel.org>, linux-aio@...ck.org,
	linux-mm <linux-mm@...ck.org>
Subject: Re: git pull -- [PATCH] aio: v2 ensure access to ctx->ring_pages is
 correctly serialised

On Thu, Mar 27, 2014 at 11:16 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> It would all be cleaner if all the setup was done with the
> ctx->ring_lock held (you can even *initialize* it to the locked state,
> since this is the function that allocates it!) and then it would just
> be unlocked when done.

Attached is a TOTALLY UNTESTED patch based on Ben's one that does at
least this minimal cleanup, in addition to dropping the
completion_lock in aio_free_ring() in favor of instead just doing the
"put_aio_ring_file()" first.

I do want to stress that "untested" part. I'm not going to commit
this, because I don't have any real test-cases even if it were to boot
and work for me otherwise.

I can't say that I like the locking. It really seems totally
mis-designed to me. For example, the first completion_lock in
aio_migratepage() seems to be total BS - it's locking against other
migrations, but that's what "mapping->private_lock" (and now
"ctx->ring_lock") protect against.

The *second* completion_lock use in aio_migratepage() is actually
valid: we can't copy the page contents to a new one when a completion
might change the ring tail data, because then the change might be done
to the old page but not the new page. But there the "check if things
haven't changed" is bogus, since we've held the ring_lock.

I did *not* clean up that part. But it's an example of how the locking
here seems to be more "voodoo programming" than actually thought about
and designed.

Please, somebody who has test-cases look at this, ok?

                  Linus

View attachment "patch.diff" of type "text/plain" (4455 bytes)