From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [PATCH] fs: aio: use correct integer overflow checks when creation aio ctx Date: Fri, 17 May 2013 15:05:50 -0400 Message-ID: <51967F8E.2070400@oracle.com> References: <1368815034-844-1-git-send-email-sasha.levin@oracle.com> <20130517185317.GM1008@kvack.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: koverstreet@google.com, akpm@linux-foundation.org, tytso@mit.edu, viro@zeniv.linux.org.uk, linux-aio@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Benjamin LaHaise Return-path: In-Reply-To: <20130517185317.GM1008@kvack.org> Sender: owner-linux-aio@kvack.org List-Id: linux-fsdevel.vger.kernel.org On 05/17/2013 02:53 PM, Benjamin LaHaise wrote: > On Fri, May 17, 2013 at 02:23:54PM -0400, Sasha Levin wrote: >> Commit "aio: percpu reqs_available" added some math to the nr_requests >> calculation, but didn't correct the overflow calculations to handle that. >> >> This means that this: >> >> #include >> void main(void) >> { >> aio_context_t ctx_idp; >> io_setup(0x80000001, &ctx_idp); >> } >> >> Would trigger the newly added BUG() couple of lines after the overflow >> checks. > > This BUG() isn't in Linus' tree, and probably should be removed before > it gets there. It's not, it's in -next though. >> Signed-off-by: Sasha Levin >> --- >> fs/aio.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/aio.c b/fs/aio.c >> index 5b7ed78..0ae450a 100644 >> --- a/fs/aio.c >> +++ b/fs/aio.c >> @@ -411,7 +411,8 @@ static struct kioctx *ioctx_alloc(unsigned nr_events) >> >> /* Prevent overflows */ >> if ((nr_events > (0x10000000U / sizeof(struct io_event))) || >> - (nr_events > (0x10000000U / sizeof(struct kiocb)))) { >> + (nr_events > (0x10000000U / sizeof(struct kiocb))) || >> + (nr_events < num_possible_cpus() * 4)) { >> pr_debug("ENOMEM: nr_events too high\n"); >> return ERR_PTR(-EINVAL); > > This is completely wrong. Enforcing a minimum needs to be done in a way > that doesn't fail for existing users that potentially use a minimum > smaller than what is newly required. That is: an existing userland program > that only requests 16 events must not fail because of changes to the kernel > that increase the minimum number of requests. So I have to NACK this patch > as it stands. You didn't look around the context of the patch. Couple of lines before that check, this happens: nr_events = max(nr_events, num_possible_cpus() * 4); nr_events *= 2; The check I've added would only make sense if nr_events wrapped around, not if nr_events was originally smaller than (num_possible_cpus() * 4). Thanks, Sasha -- To unsubscribe, send a message with 'unsubscribe linux-aio' in the body to majordomo@kvack.org. For more info on Linux AIO, see: http://www.kvack.org/aio/ Don't email: aart@kvack.org