AppArmor: Use GFP_KERNEL for __aa_kvmalloc().
diff mbox

Message ID 1479121912-13384-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp
State New
Headers show

Commit Message

Tetsuo Handa Nov. 14, 2016, 11:11 a.m. UTC
Calling kmalloc(GFP_NOIO) with order == PAGE_ALLOC_COSTLY_ORDER is not
recommended because it might fall into infinite retry loop without
invoking the OOM killer.

Since aa_dfa_unpack() is the only caller of kvzalloc() and
aa_dfa_unpack() which is calling kvzalloc() via unpack_table() is
doing kzalloc(GFP_KERNEL), it is safe to use GFP_KERNEL from
__aa_kvmalloc().

Since aa_simple_write_to_buffer() is the only caller of kvmalloc()
and aa_simple_write_to_buffer() is calling copy_from_user() which
is GFP_KERNEL context (see memdup_user_nul()), it is safe to use
GFP_KERNEL from __aa_kvmalloc().

Therefore, replace GFP_NOIO with GFP_KERNEL. Also, since we have
vmalloc() fallback, add __GFP_NORETRY so that we don't invoke the OOM
killer by kmalloc(GFP_KERNEL) with order == PAGE_ALLOC_COSTLY_ORDER.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/apparmor/lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

John Johansen Nov. 18, 2016, 1:57 a.m. UTC | #1
On 11/14/2016 03:11 AM, Tetsuo Handa wrote:
> Calling kmalloc(GFP_NOIO) with order == PAGE_ALLOC_COSTLY_ORDER is not
> recommended because it might fall into infinite retry loop without
> invoking the OOM killer.
> 
> Since aa_dfa_unpack() is the only caller of kvzalloc() and
> aa_dfa_unpack() which is calling kvzalloc() via unpack_table() is
> doing kzalloc(GFP_KERNEL), it is safe to use GFP_KERNEL from
> __aa_kvmalloc().
> 
> Since aa_simple_write_to_buffer() is the only caller of kvmalloc()
> and aa_simple_write_to_buffer() is calling copy_from_user() which
> is GFP_KERNEL context (see memdup_user_nul()), it is safe to use
> GFP_KERNEL from __aa_kvmalloc().
> 
> Therefore, replace GFP_NOIO with GFP_KERNEL. Also, since we have
> vmalloc() fallback, add __GFP_NORETRY so that we don't invoke the OOM
> killer by kmalloc(GFP_KERNEL) with order == PAGE_ALLOC_COSTLY_ORDER.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/apparmor/lib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
> index c1827e0..2ef422a 100644
> --- a/security/apparmor/lib.c
> +++ b/security/apparmor/lib.c
> @@ -95,7 +95,8 @@ void *__aa_kvmalloc(size_t size, gfp_t flags)
>  
>  	/* do not attempt kmalloc if we need more than 16 pages at once */
>  	if (size <= (16*PAGE_SIZE))
> -		buffer = kmalloc(size, flags | GFP_NOIO | __GFP_NOWARN);
> +		buffer = kmalloc(size, flags | GFP_KERNEL | __GFP_NORETRY |
> +				 __GFP_NOWARN);
>  	if (!buffer) {
>  		if (flags & __GFP_ZERO)
>  			buffer = vzalloc(size);
> 

yep, thanks Tetsu
I'll pull it into my tree and send it up

Acked-by: John Johansen <john.johansen@canonical.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
index c1827e0..2ef422a 100644
--- a/security/apparmor/lib.c
+++ b/security/apparmor/lib.c
@@ -95,7 +95,8 @@  void *__aa_kvmalloc(size_t size, gfp_t flags)
 
 	/* do not attempt kmalloc if we need more than 16 pages at once */
 	if (size <= (16*PAGE_SIZE))
-		buffer = kmalloc(size, flags | GFP_NOIO | __GFP_NOWARN);
+		buffer = kmalloc(size, flags | GFP_KERNEL | __GFP_NORETRY |
+				 __GFP_NOWARN);
 	if (!buffer) {
 		if (flags & __GFP_ZERO)
 			buffer = vzalloc(size);