linux-kernel - Revert "aio: fix aio request leak when events are reaped by user space"

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]

Date:	Tue, 19 Aug 2014 19:37:33 +0300
From:	Dan Aloni <dan@...nelim.com>
To:	Benjamin LaHaise <bcrl@...ck.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	security@...nel.org, linux-aio@...ck.org,
	linux-kernel@...r.kernel.org, Mateusz Guzik <mguzik@...hat.com>,
	Petr Matousek <pmatouse@...hat.com>,
	Kent Overstreet <kmo@...erainc.com>,
	Jeff Moyer <jmoyer@...hat.com>, stable@...r.kernel.org
Subject: Revert "aio: fix aio request leak when events are reaped by user
 space"

Some testing I've done today indicates that the original commit broke
AIO with regard to users that overflow the maximum number of request
per IO context (where -EAGAIN is returned).

In fact, it did worse - the attached C program can easily overrun the
ring buffer that is responsible for managing the completed requests,
and caused notification about their completion never to be returned.

See attachments to this mail:

    aio_bug.c:       a program that helps to reproduce the probelm
    dmesg-bad.txt:  fs/aio.c with dynamic printk on v3.15.10.
                    The printks show how the ring buffer gets overrun.
    dmesg-good.txt: fs/aio.c with dynamic printk on a kernel with the
                    commit reverted.

For this program to work, we need enough events coming from io_submit()
in one go so that the ring buffer overflows. Local file system should
support O_DIRECT and it seems to test here correctly on a laptop with
an SSD.

I am not an expert on this code, but it looks perhaps that the reason
that put_reqs_available() was called in the original context was to
prevent this scenario exactly, and to properly manage the maximum
number of outstanding requests.

This reverts commit b34e0e1319b31202eb142dcd9688cf7145a30bf6.

Signed-off-by: Dan Aloni <dan@...nelim.com>

---
 fs/aio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index 6d68e01dc7ca..45b07e8a1834 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1028,7 +1028,6 @@ void aio_complete(struct kiocb *iocb, long res, long res2)

 	/* everything turned out well, dispose of the aiocb. */
 	kiocb_free(iocb);
-	put_reqs_available(ctx, 1);

 	/*
 	 * We have to order our ring_info tail store above and test
@@ -1111,6 +1110,8 @@ static long aio_read_events_ring(struct kioctx *ctx,
 	flush_dcache_page(ctx->ring_pages[0]);

 	pr_debug("%li  h%u t%u\n", ret, head, tail);
+
+	put_reqs_available(ctx, ret);
 out:
 	mutex_unlock(&ctx->ring_lock);

-- 
1.9.3

-- 
Dan Aloni

View attachment "dmesg-bad.txt" of type "text/plain" (21518 bytes)

View attachment "dmesg-good.txt" of type "text/plain" (21492 bytes)

View attachment "aio_bug.c" of type "text/plain" (2858 bytes)