From: Benjamin LaHaise <bcrl@kvack.org>
To: Sasha Levin <sasha.levin@oracle.com>
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
Subject: Re: [PATCH] fs: aio: use correct integer overflow checks when creation aio ctx
Date: Fri, 17 May 2013 14:53:17 -0400 [thread overview]
Message-ID: <20130517185317.GM1008@kvack.org> (raw)
In-Reply-To: <1368815034-844-1-git-send-email-sasha.levin@oracle.com>
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 <linux/aio_abi.h>
> 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.
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
> 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.
-ben
> }
> --
> 1.8.2.1
--
"Thought is the essence of where you are now."
--
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: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
WARNING: multiple messages have this Message-ID (diff)
From: Benjamin LaHaise <bcrl@kvack.org>
To: Sasha Levin <sasha.levin@oracle.com>
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
Subject: Re: [PATCH] fs: aio: use correct integer overflow checks when creation aio ctx
Date: Fri, 17 May 2013 14:53:17 -0400 [thread overview]
Message-ID: <20130517185317.GM1008@kvack.org> (raw)
In-Reply-To: <1368815034-844-1-git-send-email-sasha.levin@oracle.com>
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 <linux/aio_abi.h>
> 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.
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> ---
> 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.
-ben
> }
> --
> 1.8.2.1
--
"Thought is the essence of where you are now."
next prev parent reply other threads:[~2013-05-17 18:53 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-17 18:23 [PATCH] fs: aio: use correct integer overflow checks when creation aio ctx Sasha Levin
2013-05-17 18:23 ` Sasha Levin
2013-05-17 18:53 ` Benjamin LaHaise [this message]
2013-05-17 18:53 ` Benjamin LaHaise
2013-05-17 19:05 ` Sasha Levin
2013-05-17 19:05 ` Sasha Levin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130517185317.GM1008@kvack.org \
--to=bcrl@kvack.org \
--cc=akpm@linux-foundation.org \
--cc=koverstreet@google.com \
--cc=linux-aio@kvack.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sasha.levin@oracle.com \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.