linux-kernel - Re: 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]

Message-ID: <20140822185110.GA2333@gmail.com>
Date:	Fri, 22 Aug 2014 21:51:10 +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: Re: Revert "aio: fix aio request leak when events are reaped by user
 space"

On Fri, Aug 22, 2014 at 12:26:30PM -0400, Benjamin LaHaise wrote:
> On Fri, Aug 22, 2014 at 07:15:02PM +0300, Dan Aloni wrote:
> > Sorry, I was waiting for a new patch from your direction, I should
> > have replied earlier. What bothered me about the patch you sent is that
> > completed_events is added as a new field but nothing assigns to it, so I 
> > wonder how it can be effective.
> 
> Ah, that was missing a hunk then.  Try this version instead.

Ben, seems that the test program needs some twidling to make the bug
appear still by setting MAX_IOS to 256 (and it still passes on a
kernel with the original patch reverted). Under this condition the
ring buffer size remains 128 (here, 32*4 CPUs), and it is overrun on
the second attempt.

    $ ./aio_bug 
    Submitting: 256
    Submitted: 126
    Submitting: 130
    Submitted too much, that's okay
    Completed: 126
    Submitting: 130
    Submitted: 130
    <stuck>

I think I have found two problems with your patch: first, the
completed_events field is never decremented so it goes up until 2^32
wraparound. So I tested with 'ctx->completed_events -= completed;'
there (along with some prints), but testing revealed that this didn't
solve the problem, so secondly, I also fixed the 'avail = ' line. The
case where the 'head > tail' case didn't look correct to me.

So the good news is that it works now with fix below and MAX_IOS=256
and even with MAX_IOS=512. You can git-amend this it to your patch I
guess.

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

diff --git a/fs/aio.c b/fs/aio.c
index 6982357d9372..eafc96c60a8c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -893,12 +893,20 @@ static void refill_reqs_available(struct kioctx *ctx)
 		tail = ACCESS_ONCE(ring->tail);
 		kunmap_atomic(ring);

-		avail = (head <= tail ?  tail : ctx->nr_events) - head;
+		if (head <= tail)
+			avail = tail - head;
+		else
+			avail = ctx->nr_events - (head - tail);
+
 		completed = ctx->completed_events;
+		pr_debug("%u %u  h%u t%u\n", avail, completed, head, tail);
 		if (avail < completed)
 			completed -= avail;
 		else
 			completed = 0;
+		pr_debug("completed %u\n", completed);
+
+		ctx->completed_events -= completed;
 		put_reqs_available(ctx, completed);
 	}

BTW, I am not an expert on this code so I am still not sure that
'ctx->completed_events' wouldn't get wrong if for instance - one SMP
core does userspace event reaping and another calls io_submit(). Do
you think it would work alright in that case?

-- 
Dan Aloni
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/