suspicious __GFP_NOMEMALLOC in selinux

suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

Hi,
while doing something completely unrelated to selinux I've noticed a
really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
on its own allows to access memory reserves while the later flag tells
we cannot use memory reserves at all. The primary usecase for
__GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
need.

It all leads to fa1aa143ac4a ("selinux: extended permissions for
ioctls") which doesn't explain this aspect so let me ask. Why is the
flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
What makes this path important to access memory reserves?

Thanks
-- 
Michal Hocko
SUSE Labs

suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

Hi,
while doing something completely unrelated to selinux I've noticed a
really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
on its own allows to access memory reserves while the later flag tells
we cannot use memory reserves at all. The primary usecase for
__GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
need.

It all leads to fa1aa143ac4a ("selinux: extended permissions for
ioctls") which doesn't explain this aspect so let me ask. Why is the
flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
What makes this path important to access memory reserves?

Thanks
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> Hi,
> while doing something completely unrelated to selinux I've noticed a
> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> on its own allows to access memory reserves while the later flag tells
> we cannot use memory reserves at all. The primary usecase for
> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> need.
>
> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> ioctls") which doesn't explain this aspect so let me ask. Why is the
> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> What makes this path important to access memory reserves?

[NOTE: added the SELinux list to the CC line, please include that list
when asking SELinux questions]

The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
to security/selinux/avc.c, and digging a bit, I'm guessing commit
fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
avc cache alloc as non-critical") and the avc_alloc_node() function.

I can't say that I'm an expert at the vm subsystem and the variety of
different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
security/selinux/avc.c seems reasonable and in keeping with the idea
behind commit 6290c2c43973.

-- 
paul moore
security @ redhat

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> Hi,
> while doing something completely unrelated to selinux I've noticed a
> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> on its own allows to access memory reserves while the later flag tells
> we cannot use memory reserves at all. The primary usecase for
> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> need.
>
> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> ioctls") which doesn't explain this aspect so let me ask. Why is the
> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> What makes this path important to access memory reserves?

[NOTE: added the SELinux list to the CC line, please include that list
when asking SELinux questions]

The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
to security/selinux/avc.c, and digging a bit, I'm guessing commit
fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
avc cache alloc as non-critical") and the avc_alloc_node() function.

I can't say that I'm an expert at the vm subsystem and the variety of
different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
security/selinux/avc.c seems reasonable and in keeping with the idea
behind commit 6290c2c43973.

-- 
paul moore
security @ redhat

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

[CC Mel]

On Wed 02-08-17 17:45:56, Paul Moore wrote:
> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > Hi,
> > while doing something completely unrelated to selinux I've noticed a
> > really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > on its own allows to access memory reserves while the later flag tells
> > we cannot use memory reserves at all. The primary usecase for
> > __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > need.
> >
> > It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > ioctls") which doesn't explain this aspect so let me ask. Why is the
> > flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > What makes this path important to access memory reserves?
> 
> [NOTE: added the SELinux list to the CC line, please include that list
> when asking SELinux questions]

Sorry about that. Will keep it in mind for next posts
 
> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> avc cache alloc as non-critical") and the avc_alloc_node() function.

Thanks for the pointer. That makes much more sense now. Back in 2012 we
really didn't have a good way to distinguish non sleeping and atomic
with reserves allocations.
 
> I can't say that I'm an expert at the vm subsystem and the variety of
> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> security/selinux/avc.c seems reasonable and in keeping with the idea
> behind commit 6290c2c43973.

What do you think about the following? I haven't tested it but it should
be rather straightforward.
---
>From 6d506a75da83c0724ed399966971711f37d67411 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 3 Aug 2017 10:04:20 +0200
Subject: [PATCH] selinux: replace GFP_ATOMIC | __GFP_NOMEMALLOC with
 GFP_NOWAIT

selinux avc code uses a weird combination of gfp flags. It asks for
GFP_ATOMIC which allows access to memory reserves while it requests
to not consume memory reserves by __GFP_NOMEMALLOC. This seems to be
copying the pattern from 6290c2c43973 ("selinux: tag avc cache alloc as
non-critical").

Back then (before d0164adc89f6 ("mm, page_alloc: distinguish between
being unable to sleep, unwilling to sleep and avoiding waking kswapd"))
we didn't have a good way to distinguish nowait and atomic allocations
so the construct made some sense. Now we do not have to play tricks
though and GFP_NOWAIT will provide the required semantic (aka do not
sleep and do not consume any memory reserves).

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 security/selinux/avc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 4b4293194aee..b2c159e832b6 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -346,27 +346,26 @@ static struct avc_xperms_decision_node
 	struct avc_xperms_decision_node *xpd_node;
 	struct extended_perms_decision *xpd;
 
-	xpd_node = kmem_cache_zalloc(avc_xperms_decision_cachep,
-				GFP_ATOMIC | __GFP_NOMEMALLOC);
+	xpd_node = kmem_cache_zalloc(avc_xperms_decision_cachep, GFP_NOWAIT);
 	if (!xpd_node)
 		return NULL;
 
 	xpd = &xpd_node->xpd;
 	if (which & XPERMS_ALLOWED) {
 		xpd->allowed = kmem_cache_zalloc(avc_xperms_data_cachep,
-						GFP_ATOMIC | __GFP_NOMEMALLOC);
+						GFP_NOWAIT);
 		if (!xpd->allowed)
 			goto error;
 	}
 	if (which & XPERMS_AUDITALLOW) {
 		xpd->auditallow = kmem_cache_zalloc(avc_xperms_data_cachep,
-						GFP_ATOMIC | __GFP_NOMEMALLOC);
+						GFP_NOWAIT);
 		if (!xpd->auditallow)
 			goto error;
 	}
 	if (which & XPERMS_DONTAUDIT) {
 		xpd->dontaudit = kmem_cache_zalloc(avc_xperms_data_cachep,
-						GFP_ATOMIC | __GFP_NOMEMALLOC);
+						GFP_NOWAIT);
 		if (!xpd->dontaudit)
 			goto error;
 	}
@@ -394,8 +393,7 @@ static struct avc_xperms_node *avc_xperms_alloc(void)
 {
 	struct avc_xperms_node *xp_node;
 
-	xp_node = kmem_cache_zalloc(avc_xperms_cachep,
-				GFP_ATOMIC|__GFP_NOMEMALLOC);
+	xp_node = kmem_cache_zalloc(avc_xperms_cachep, GFP_NOWAIT);
 	if (!xp_node)
 		return xp_node;
 	INIT_LIST_HEAD(&xp_node->xpd_head);
@@ -548,7 +546,7 @@ static struct avc_node *avc_alloc_node(void)
 {
 	struct avc_node *node;
 
-	node = kmem_cache_zalloc(avc_node_cachep, GFP_ATOMIC|__GFP_NOMEMALLOC);
+	node = kmem_cache_zalloc(avc_node_cachep, GFP_NOWAIT);
 	if (!node)
 		goto out;
 
-- 
2.13.2

-- 
Michal Hocko
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

[CC Mel]

On Wed 02-08-17 17:45:56, Paul Moore wrote:
> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > Hi,
> > while doing something completely unrelated to selinux I've noticed a
> > really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > on its own allows to access memory reserves while the later flag tells
> > we cannot use memory reserves at all. The primary usecase for
> > __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > need.
> >
> > It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > ioctls") which doesn't explain this aspect so let me ask. Why is the
> > flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > What makes this path important to access memory reserves?
> 
> [NOTE: added the SELinux list to the CC line, please include that list
> when asking SELinux questions]

Sorry about that. Will keep it in mind for next posts
 
> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> avc cache alloc as non-critical") and the avc_alloc_node() function.

Thanks for the pointer. That makes much more sense now. Back in 2012 we
really didn't have a good way to distinguish non sleeping and atomic
with reserves allocations.
 
> I can't say that I'm an expert at the vm subsystem and the variety of
> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> security/selinux/avc.c seems reasonable and in keeping with the idea
> behind commit 6290c2c43973.

What do you think about the following? I haven't tested it but it should
be rather straightforward.
---

Re: suspicious __GFP_NOMEMALLOC in selinux

[Mel Gorman]

On Thu, Aug 03, 2017 at 10:11:52AM +0200, Michal Hocko wrote:
> > The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > avc cache alloc as non-critical") and the avc_alloc_node() function.
> 
> Thanks for the pointer. That makes much more sense now. Back in 2012 we
> really didn't have a good way to distinguish non sleeping and atomic
> with reserves allocations.
>  

Yes, and GFP_NOWAIT is the right way to express that now. It'll still
wake kswapd but it won't stall on direct reclaim and won't dip into
reserves.

Thanks Michal.

> ---
> From 6d506a75da83c0724ed399966971711f37d67411 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 3 Aug 2017 10:04:20 +0200
> Subject: [PATCH] selinux: replace GFP_ATOMIC | __GFP_NOMEMALLOC with
>  GFP_NOWAIT
> 
> selinux avc code uses a weird combination of gfp flags. It asks for
> GFP_ATOMIC which allows access to memory reserves while it requests
> to not consume memory reserves by __GFP_NOMEMALLOC. This seems to be
> copying the pattern from 6290c2c43973 ("selinux: tag avc cache alloc as
> non-critical").
> 
> Back then (before d0164adc89f6 ("mm, page_alloc: distinguish between
> being unable to sleep, unwilling to sleep and avoiding waking kswapd"))
> we didn't have a good way to distinguish nowait and atomic allocations
> so the construct made some sense. Now we do not have to play tricks
> though and GFP_NOWAIT will provide the required semantic (aka do not
> sleep and do not consume any memory reserves).
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Looks right to me

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Mel Gorman]

On Thu, Aug 03, 2017 at 10:11:52AM +0200, Michal Hocko wrote:
> > The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > avc cache alloc as non-critical") and the avc_alloc_node() function.
> 
> Thanks for the pointer. That makes much more sense now. Back in 2012 we
> really didn't have a good way to distinguish non sleeping and atomic
> with reserves allocations.
>  

Yes, and GFP_NOWAIT is the right way to express that now. It'll still
wake kswapd but it won't stall on direct reclaim and won't dip into
reserves.

Thanks Michal.

> ---
> From 6d506a75da83c0724ed399966971711f37d67411 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 3 Aug 2017 10:04:20 +0200
> Subject: [PATCH] selinux: replace GFP_ATOMIC | __GFP_NOMEMALLOC with
>  GFP_NOWAIT
> 
> selinux avc code uses a weird combination of gfp flags. It asks for
> GFP_ATOMIC which allows access to memory reserves while it requests
> to not consume memory reserves by __GFP_NOMEMALLOC. This seems to be
> copying the pattern from 6290c2c43973 ("selinux: tag avc cache alloc as
> non-critical").
> 
> Back then (before d0164adc89f6 ("mm, page_alloc: distinguish between
> being unable to sleep, unwilling to sleep and avoiding waking kswapd"))
> we didn't have a good way to distinguish nowait and atomic allocations
> so the construct made some sense. Now we do not have to play tricks
> though and GFP_NOWAIT will provide the required semantic (aka do not
> sleep and do not consume any memory reserves).
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Looks right to me

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Tetsuo Handa]

On 2017/08/03 17:11, Michal Hocko wrote:
> [CC Mel]
> 
> On Wed 02-08-17 17:45:56, Paul Moore wrote:
>> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
>>> Hi,
>>> while doing something completely unrelated to selinux I've noticed a
>>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
>>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
>>> on its own allows to access memory reserves while the later flag tells
>>> we cannot use memory reserves at all. The primary usecase for
>>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
>>> need.
>>>
>>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
>>> ioctls") which doesn't explain this aspect so let me ask. Why is the
>>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
>>> What makes this path important to access memory reserves?
>>
>> [NOTE: added the SELinux list to the CC line, please include that list
>> when asking SELinux questions]
> 
> Sorry about that. Will keep it in mind for next posts
>  
>> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
>> to security/selinux/avc.c, and digging a bit, I'm guessing commit
>> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
>> avc cache alloc as non-critical") and the avc_alloc_node() function.
> 
> Thanks for the pointer. That makes much more sense now. Back in 2012 we
> really didn't have a good way to distinguish non sleeping and atomic
> with reserves allocations.
>  
>> I can't say that I'm an expert at the vm subsystem and the variety of
>> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
>> security/selinux/avc.c seems reasonable and in keeping with the idea
>> behind commit 6290c2c43973.
> 
> What do you think about the following? I haven't tested it but it should
> be rather straightforward.

Why not at least __GFP_NOWARN ? And why not also __GFP_NOMEMALLOC ?
http://lkml.kernel.org/r/201706302210.GCA05089.MFFOtQVJSOLHOF@I-love.SAKURA.ne.jp

Re: suspicious __GFP_NOMEMALLOC in selinux

[Tetsuo Handa]

On 2017/08/03 17:11, Michal Hocko wrote:
> [CC Mel]
> 
> On Wed 02-08-17 17:45:56, Paul Moore wrote:
>> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
>>> Hi,
>>> while doing something completely unrelated to selinux I've noticed a
>>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
>>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
>>> on its own allows to access memory reserves while the later flag tells
>>> we cannot use memory reserves at all. The primary usecase for
>>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
>>> need.
>>>
>>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
>>> ioctls") which doesn't explain this aspect so let me ask. Why is the
>>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
>>> What makes this path important to access memory reserves?
>>
>> [NOTE: added the SELinux list to the CC line, please include that list
>> when asking SELinux questions]
> 
> Sorry about that. Will keep it in mind for next posts
>  
>> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
>> to security/selinux/avc.c, and digging a bit, I'm guessing commit
>> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
>> avc cache alloc as non-critical") and the avc_alloc_node() function.
> 
> Thanks for the pointer. That makes much more sense now. Back in 2012 we
> really didn't have a good way to distinguish non sleeping and atomic
> with reserves allocations.
>  
>> I can't say that I'm an expert at the vm subsystem and the variety of
>> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
>> security/selinux/avc.c seems reasonable and in keeping with the idea
>> behind commit 6290c2c43973.
> 
> What do you think about the following? I haven't tested it but it should
> be rather straightforward.

Why not at least __GFP_NOWARN ? And why not also __GFP_NOMEMALLOC ?
http://lkml.kernel.org/r/201706302210.GCA05089.MFFOtQVJSOLHOF@I-love.SAKURA.ne.jp

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> On 2017/08/03 17:11, Michal Hocko wrote:
> > [CC Mel]
> > 
> > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >>> Hi,
> >>> while doing something completely unrelated to selinux I've noticed a
> >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> >>> on its own allows to access memory reserves while the later flag tells
> >>> we cannot use memory reserves at all. The primary usecase for
> >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> >>> need.
> >>>
> >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> >>> What makes this path important to access memory reserves?
> >>
> >> [NOTE: added the SELinux list to the CC line, please include that list
> >> when asking SELinux questions]
> > 
> > Sorry about that. Will keep it in mind for next posts
> >  
> >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > 
> > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > really didn't have a good way to distinguish non sleeping and atomic
> > with reserves allocations.
> >  
> >> I can't say that I'm an expert at the vm subsystem and the variety of
> >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> >> security/selinux/avc.c seems reasonable and in keeping with the idea
> >> behind commit 6290c2c43973.
> > 
> > What do you think about the following? I haven't tested it but it should
> > be rather straightforward.
> 
> Why not at least __GFP_NOWARN ?

This would require an additional justification.

> And why not also __GFP_NOMEMALLOC ?

What would be the purpose of __GFP_NOMEMALLOC? In other words which
context would set PF_NOMEMALLOC so that the flag would override it?

-- 
Michal Hocko
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> On 2017/08/03 17:11, Michal Hocko wrote:
> > [CC Mel]
> > 
> > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> >>> Hi,
> >>> while doing something completely unrelated to selinux I've noticed a
> >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> >>> on its own allows to access memory reserves while the later flag tells
> >>> we cannot use memory reserves at all. The primary usecase for
> >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> >>> need.
> >>>
> >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> >>> What makes this path important to access memory reserves?
> >>
> >> [NOTE: added the SELinux list to the CC line, please include that list
> >> when asking SELinux questions]
> > 
> > Sorry about that. Will keep it in mind for next posts
> >  
> >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > 
> > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > really didn't have a good way to distinguish non sleeping and atomic
> > with reserves allocations.
> >  
> >> I can't say that I'm an expert at the vm subsystem and the variety of
> >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> >> security/selinux/avc.c seems reasonable and in keeping with the idea
> >> behind commit 6290c2c43973.
> > 
> > What do you think about the following? I haven't tested it but it should
> > be rather straightforward.
> 
> Why not at least __GFP_NOWARN ?

This would require an additional justification.

> And why not also __GFP_NOMEMALLOC ?

What would be the purpose of __GFP_NOMEMALLOC? In other words which
context would set PF_NOMEMALLOC so that the flag would override it?

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Tetsuo Handa]

Michal Hocko wrote:
> On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> > On 2017/08/03 17:11, Michal Hocko wrote:
> > > [CC Mel]
> > > 
> > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > >>> Hi,
> > >>> while doing something completely unrelated to selinux I've noticed a
> > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > >>> on its own allows to access memory reserves while the later flag tells
> > >>> we cannot use memory reserves at all. The primary usecase for
> > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > >>> need.
> > >>>
> > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > >>> What makes this path important to access memory reserves?
> > >>
> > >> [NOTE: added the SELinux list to the CC line, please include that list
> > >> when asking SELinux questions]
> > > 
> > > Sorry about that. Will keep it in mind for next posts
> > >  
> > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > > 
> > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > > really didn't have a good way to distinguish non sleeping and atomic
> > > with reserves allocations.
> > >  
> > >> I can't say that I'm an expert at the vm subsystem and the variety of
> > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> > >> security/selinux/avc.c seems reasonable and in keeping with the idea
> > >> behind commit 6290c2c43973.
> > > 
> > > What do you think about the following? I haven't tested it but it should
> > > be rather straightforward.
> > 
> > Why not at least __GFP_NOWARN ?
> 
> This would require an additional justification.

If allocation failure is not a problem, printing allocation failure messages
is nothing but noisy.

> 
> > And why not also __GFP_NOMEMALLOC ?
> 
> What would be the purpose of __GFP_NOMEMALLOC? In other words which
> context would set PF_NOMEMALLOC so that the flag would override it?
> 

When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
Since that function might be called from !in_interrupt() context, it is
possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
the OOM victim will dip into memory reserves even when allocation failure
is not a problem.

Thus, I think that majority of plain GFP_NOWAIT users want to use
GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

Re: suspicious __GFP_NOMEMALLOC in selinux

[Tetsuo Handa]

Michal Hocko wrote:
> On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> > On 2017/08/03 17:11, Michal Hocko wrote:
> > > [CC Mel]
> > > 
> > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > >>> Hi,
> > >>> while doing something completely unrelated to selinux I've noticed a
> > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > >>> on its own allows to access memory reserves while the later flag tells
> > >>> we cannot use memory reserves at all. The primary usecase for
> > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > >>> need.
> > >>>
> > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > >>> What makes this path important to access memory reserves?
> > >>
> > >> [NOTE: added the SELinux list to the CC line, please include that list
> > >> when asking SELinux questions]
> > > 
> > > Sorry about that. Will keep it in mind for next posts
> > >  
> > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > > 
> > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > > really didn't have a good way to distinguish non sleeping and atomic
> > > with reserves allocations.
> > >  
> > >> I can't say that I'm an expert at the vm subsystem and the variety of
> > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> > >> security/selinux/avc.c seems reasonable and in keeping with the idea
> > >> behind commit 6290c2c43973.
> > > 
> > > What do you think about the following? I haven't tested it but it should
> > > be rather straightforward.
> > 
> > Why not at least __GFP_NOWARN ?
> 
> This would require an additional justification.

If allocation failure is not a problem, printing allocation failure messages
is nothing but noisy.

> 
> > And why not also __GFP_NOMEMALLOC ?
> 
> What would be the purpose of __GFP_NOMEMALLOC? In other words which
> context would set PF_NOMEMALLOC so that the flag would override it?
> 

When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
Since that function might be called from !in_interrupt() context, it is
possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
the OOM victim will dip into memory reserves even when allocation failure
is not a problem.

Thus, I think that majority of plain GFP_NOWAIT users want to use
GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> > > On 2017/08/03 17:11, Michal Hocko wrote:
> > > > [CC Mel]
> > > > 
> > > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> > > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > >>> Hi,
> > > >>> while doing something completely unrelated to selinux I've noticed a
> > > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > > >>> on its own allows to access memory reserves while the later flag tells
> > > >>> we cannot use memory reserves at all. The primary usecase for
> > > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > > >>> need.
> > > >>>
> > > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> > > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > > >>> What makes this path important to access memory reserves?
> > > >>
> > > >> [NOTE: added the SELinux list to the CC line, please include that list
> > > >> when asking SELinux questions]
> > > > 
> > > > Sorry about that. Will keep it in mind for next posts
> > > >  
> > > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > > > 
> > > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > > > really didn't have a good way to distinguish non sleeping and atomic
> > > > with reserves allocations.
> > > >  
> > > >> I can't say that I'm an expert at the vm subsystem and the variety of
> > > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> > > >> security/selinux/avc.c seems reasonable and in keeping with the idea
> > > >> behind commit 6290c2c43973.
> > > > 
> > > > What do you think about the following? I haven't tested it but it should
> > > > be rather straightforward.
> > > 
> > > Why not at least __GFP_NOWARN ?
> > 
> > This would require an additional justification.
> 
> If allocation failure is not a problem, printing allocation failure messages
> is nothing but noisy.

That alone is not a sufficient justification. An allocation warning
might still tell you that something is not configured properly. Note
that I am not objecting that __GFP_NOWARN is wrong it should just not be
added blindly withtout deep understanding of the code which I do not
have.

> > > And why not also __GFP_NOMEMALLOC ?
> > 
> > What would be the purpose of __GFP_NOMEMALLOC? In other words which
> > context would set PF_NOMEMALLOC so that the flag would override it?
> > 
> 
> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
> Since that function might be called from !in_interrupt() context, it is
> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
> the OOM victim will dip into memory reserves even when allocation failure
> is not a problem.

Yes this is possible but I do not see any major problem with that.
I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
sort that could be abused.

> Thus, I think that majority of plain GFP_NOWAIT users want to use
> GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

-- 
Michal Hocko
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
> > > On 2017/08/03 17:11, Michal Hocko wrote:
> > > > [CC Mel]
> > > > 
> > > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
> > > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > > >>> Hi,
> > > >>> while doing something completely unrelated to selinux I've noticed a
> > > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
> > > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
> > > >>> on its own allows to access memory reserves while the later flag tells
> > > >>> we cannot use memory reserves at all. The primary usecase for
> > > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
> > > >>> need.
> > > >>>
> > > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
> > > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
> > > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
> > > >>> What makes this path important to access memory reserves?
> > > >>
> > > >> [NOTE: added the SELinux list to the CC line, please include that list
> > > >> when asking SELinux questions]
> > > > 
> > > > Sorry about that. Will keep it in mind for next posts
> > > >  
> > > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
> > > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
> > > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
> > > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
> > > > 
> > > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
> > > > really didn't have a good way to distinguish non sleeping and atomic
> > > > with reserves allocations.
> > > >  
> > > >> I can't say that I'm an expert at the vm subsystem and the variety of
> > > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
> > > >> security/selinux/avc.c seems reasonable and in keeping with the idea
> > > >> behind commit 6290c2c43973.
> > > > 
> > > > What do you think about the following? I haven't tested it but it should
> > > > be rather straightforward.
> > > 
> > > Why not at least __GFP_NOWARN ?
> > 
> > This would require an additional justification.
> 
> If allocation failure is not a problem, printing allocation failure messages
> is nothing but noisy.

That alone is not a sufficient justification. An allocation warning
might still tell you that something is not configured properly. Note
that I am not objecting that __GFP_NOWARN is wrong it should just not be
added blindly withtout deep understanding of the code which I do not
have.

> > > And why not also __GFP_NOMEMALLOC ?
> > 
> > What would be the purpose of __GFP_NOMEMALLOC? In other words which
> > context would set PF_NOMEMALLOC so that the flag would override it?
> > 
> 
> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
> Since that function might be called from !in_interrupt() context, it is
> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
> the OOM victim will dip into memory reserves even when allocation failure
> is not a problem.

Yes this is possible but I do not see any major problem with that.
I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
sort that could be abused.

> Thus, I think that majority of plain GFP_NOWAIT users want to use
> GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
>> Michal Hocko wrote:
>> > On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
>> > > On 2017/08/03 17:11, Michal Hocko wrote:
>> > > > [CC Mel]
>> > > >
>> > > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
>> > > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > > >>> Hi,
>> > > >>> while doing something completely unrelated to selinux I've noticed a
>> > > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
>> > > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
>> > > >>> on its own allows to access memory reserves while the later flag tells
>> > > >>> we cannot use memory reserves at all. The primary usecase for
>> > > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
>> > > >>> need.
>> > > >>>
>> > > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
>> > > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
>> > > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
>> > > >>> What makes this path important to access memory reserves?
>> > > >>
>> > > >> [NOTE: added the SELinux list to the CC line, please include that list
>> > > >> when asking SELinux questions]
>> > > >
>> > > > Sorry about that. Will keep it in mind for next posts
>> > > >
>> > > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
>> > > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
>> > > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
>> > > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
>> > > >
>> > > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
>> > > > really didn't have a good way to distinguish non sleeping and atomic
>> > > > with reserves allocations.
>> > > >
>> > > >> I can't say that I'm an expert at the vm subsystem and the variety of
>> > > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
>> > > >> security/selinux/avc.c seems reasonable and in keeping with the idea
>> > > >> behind commit 6290c2c43973.
>> > > >
>> > > > What do you think about the following? I haven't tested it but it should
>> > > > be rather straightforward.
>> > >
>> > > Why not at least __GFP_NOWARN ?
>> >
>> > This would require an additional justification.
>>
>> If allocation failure is not a problem, printing allocation failure messages
>> is nothing but noisy.
>
> That alone is not a sufficient justification. An allocation warning
> might still tell you that something is not configured properly. Note
> that I am not objecting that __GFP_NOWARN is wrong it should just not be
> added blindly withtout deep understanding of the code which I do not
> have.

I understand the concern about noise from failed memory allocations,
but I tend to agree with the argument that notification of these
failures could be useful to admins/devs who are trying to diagnose
problems; let's *not* use __GFP_NOWARN in the SELinux AVC code.

>> > > And why not also __GFP_NOMEMALLOC ?
>> >
>> > What would be the purpose of __GFP_NOMEMALLOC? In other words which
>> > context would set PF_NOMEMALLOC so that the flag would override it?
>> >
>>
>> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
>> Since that function might be called from !in_interrupt() context, it is
>> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
>> the OOM victim will dip into memory reserves even when allocation failure
>> is not a problem.
>
> Yes this is possible but I do not see any major problem with that.
> I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
> sort that could be abused.

Adding __GFP_NOMEMALLOC would not hurt anything would it?

>> Thus, I think that majority of plain GFP_NOWAIT users want to use
>> GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

-- 
paul moore
www.paul-moore.com

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
>> Michal Hocko wrote:
>> > On Thu 03-08-17 19:02:57, Tetsuo Handa wrote:
>> > > On 2017/08/03 17:11, Michal Hocko wrote:
>> > > > [CC Mel]
>> > > >
>> > > > On Wed 02-08-17 17:45:56, Paul Moore wrote:
>> > > >> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > > >>> Hi,
>> > > >>> while doing something completely unrelated to selinux I've noticed a
>> > > >>> really strange __GFP_NOMEMALLOC usage pattern in selinux, especially
>> > > >>> GFP_ATOMIC | __GFP_NOMEMALLOC doesn't make much sense to me. GFP_ATOMIC
>> > > >>> on its own allows to access memory reserves while the later flag tells
>> > > >>> we cannot use memory reserves at all. The primary usecase for
>> > > >>> __GFP_NOMEMALLOC is to override a global PF_MEMALLOC should there be a
>> > > >>> need.
>> > > >>>
>> > > >>> It all leads to fa1aa143ac4a ("selinux: extended permissions for
>> > > >>> ioctls") which doesn't explain this aspect so let me ask. Why is the
>> > > >>> flag used at all? Moreover shouldn't GFP_ATOMIC be actually GFP_NOWAIT.
>> > > >>> What makes this path important to access memory reserves?
>> > > >>
>> > > >> [NOTE: added the SELinux list to the CC line, please include that list
>> > > >> when asking SELinux questions]
>> > > >
>> > > > Sorry about that. Will keep it in mind for next posts
>> > > >
>> > > >> The GFP_ATOMIC|__GFP_NOMEMALLOC use in SELinux appears to be limited
>> > > >> to security/selinux/avc.c, and digging a bit, I'm guessing commit
>> > > >> fa1aa143ac4a copied the combination from 6290c2c43973 ("selinux: tag
>> > > >> avc cache alloc as non-critical") and the avc_alloc_node() function.
>> > > >
>> > > > Thanks for the pointer. That makes much more sense now. Back in 2012 we
>> > > > really didn't have a good way to distinguish non sleeping and atomic
>> > > > with reserves allocations.
>> > > >
>> > > >> I can't say that I'm an expert at the vm subsystem and the variety of
>> > > >> different GFP_* flags, but your suggestion of moving to GFP_NOWAIT in
>> > > >> security/selinux/avc.c seems reasonable and in keeping with the idea
>> > > >> behind commit 6290c2c43973.
>> > > >
>> > > > What do you think about the following? I haven't tested it but it should
>> > > > be rather straightforward.
>> > >
>> > > Why not at least __GFP_NOWARN ?
>> >
>> > This would require an additional justification.
>>
>> If allocation failure is not a problem, printing allocation failure messages
>> is nothing but noisy.
>
> That alone is not a sufficient justification. An allocation warning
> might still tell you that something is not configured properly. Note
> that I am not objecting that __GFP_NOWARN is wrong it should just not be
> added blindly withtout deep understanding of the code which I do not
> have.

I understand the concern about noise from failed memory allocations,
but I tend to agree with the argument that notification of these
failures could be useful to admins/devs who are trying to diagnose
problems; let's *not* use __GFP_NOWARN in the SELinux AVC code.

>> > > And why not also __GFP_NOMEMALLOC ?
>> >
>> > What would be the purpose of __GFP_NOMEMALLOC? In other words which
>> > context would set PF_NOMEMALLOC so that the flag would override it?
>> >
>>
>> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
>> Since that function might be called from !in_interrupt() context, it is
>> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
>> the OOM victim will dip into memory reserves even when allocation failure
>> is not a problem.
>
> Yes this is possible but I do not see any major problem with that.
> I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
> sort that could be abused.

Adding __GFP_NOMEMALLOC would not hurt anything would it?

>> Thus, I think that majority of plain GFP_NOWAIT users want to use
>> GFP_NOWAIT | __GFP_NOWARN | __GFP_NOMEMALLOC.

-- 
paul moore
www.paul-moore.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Thu 03-08-17 14:17:26, Paul Moore wrote:
> On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
[...]
> >> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
> >> Since that function might be called from !in_interrupt() context, it is
> >> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
> >> the OOM victim will dip into memory reserves even when allocation failure
> >> is not a problem.
> >
> > Yes this is possible but I do not see any major problem with that.
> > I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
> > sort that could be abused.
> 
> Adding __GFP_NOMEMALLOC would not hurt anything would it?

I is not harmfull but I fail to see how it would be useful either and as
such it just adds a pointless gfp flag and confusion to whoever tries to
modify the code in future. Really the main purpose of __GFP_NOMEMALLOC
is to override the process scope PF_MEMALLOC. As such it is quite a hack
and the fewer users we have the better.

Btw. Should I resend the patch or somebody will take it from this email
thread?
-- 
Michal Hocko
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Thu 03-08-17 14:17:26, Paul Moore wrote:
> On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
[...]
> >> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
> >> Since that function might be called from !in_interrupt() context, it is
> >> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
> >> the OOM victim will dip into memory reserves even when allocation failure
> >> is not a problem.
> >
> > Yes this is possible but I do not see any major problem with that.
> > I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
> > sort that could be abused.
> 
> Adding __GFP_NOMEMALLOC would not hurt anything would it?

I is not harmfull but I fail to see how it would be useful either and as
such it just adds a pointless gfp flag and confusion to whoever tries to
modify the code in future. Really the main purpose of __GFP_NOMEMALLOC
is to override the process scope PF_MEMALLOC. As such it is quite a hack
and the fewer users we have the better.

Btw. Should I resend the patch or somebody will take it from this email
thread?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 03-08-17 14:17:26, Paul Moore wrote:
>> On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
> [...]
>> >> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
>> >> Since that function might be called from !in_interrupt() context, it is
>> >> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
>> >> the OOM victim will dip into memory reserves even when allocation failure
>> >> is not a problem.
>> >
>> > Yes this is possible but I do not see any major problem with that.
>> > I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
>> > sort that could be abused.
>>
>> Adding __GFP_NOMEMALLOC would not hurt anything would it?
>
> I is not harmfull but I fail to see how it would be useful either and as
> such it just adds a pointless gfp flag and confusion to whoever tries to
> modify the code in future. Really the main purpose of __GFP_NOMEMALLOC
> is to override the process scope PF_MEMALLOC. As such it is quite a hack
> and the fewer users we have the better.

Okay, that is a viable explanation for me.

> Btw. Should I resend the patch or somebody will take it from this email
> thread?

No, unless your mailer mangled the patch I should be able to pull it
from this thread.  However, I'm probably going to let this sit until
early next week on the odd chance that anyone else wants to comment on
the flag choice.  I'll send another reply once I merge the patch.

-- 
paul moore
www.paul-moore.com

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 03-08-17 14:17:26, Paul Moore wrote:
>> On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Thu 03-08-17 19:44:46, Tetsuo Handa wrote:
> [...]
>> >> When allocating thread is selected as an OOM victim, it gets TIF_MEMDIE.
>> >> Since that function might be called from !in_interrupt() context, it is
>> >> possible that gfp_pfmemalloc_allowed() returns true due to TIF_MEMDIE and
>> >> the OOM victim will dip into memory reserves even when allocation failure
>> >> is not a problem.
>> >
>> > Yes this is possible but I do not see any major problem with that.
>> > I wouldn't add __GFP_NOMEMALLOC unless there is a real runaway of some
>> > sort that could be abused.
>>
>> Adding __GFP_NOMEMALLOC would not hurt anything would it?
>
> I is not harmfull but I fail to see how it would be useful either and as
> such it just adds a pointless gfp flag and confusion to whoever tries to
> modify the code in future. Really the main purpose of __GFP_NOMEMALLOC
> is to override the process scope PF_MEMALLOC. As such it is quite a hack
> and the fewer users we have the better.

Okay, that is a viable explanation for me.

> Btw. Should I resend the patch or somebody will take it from this email
> thread?

No, unless your mailer mangled the patch I should be able to pull it
from this thread.  However, I'm probably going to let this sit until
early next week on the odd chance that anyone else wants to comment on
the flag choice.  I'll send another reply once I merge the patch.

-- 
paul moore
www.paul-moore.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Fri 04-08-17 13:12:04, Paul Moore wrote:
> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > Btw. Should I resend the patch or somebody will take it from this email
> > thread?
> 
> No, unless your mailer mangled the patch I should be able to pull it
> from this thread.  However, I'm probably going to let this sit until
> early next week on the odd chance that anyone else wants to comment on
> the flag choice.  I'll send another reply once I merge the patch.

OK, there is certainly no hurry for merging this. Thanks!
-- 
Michal Hocko
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Fri 04-08-17 13:12:04, Paul Moore wrote:
> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > Btw. Should I resend the patch or somebody will take it from this email
> > thread?
> 
> No, unless your mailer mangled the patch I should be able to pull it
> from this thread.  However, I'm probably going to let this sit until
> early next week on the odd chance that anyone else wants to comment on
> the flag choice.  I'll send another reply once I merge the patch.

OK, there is certainly no hurry for merging this. Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 04-08-17 13:12:04, Paul Moore wrote:
>> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> [...]
>> > Btw. Should I resend the patch or somebody will take it from this email
>> > thread?
>>
>> No, unless your mailer mangled the patch I should be able to pull it
>> from this thread.  However, I'm probably going to let this sit until
>> early next week on the odd chance that anyone else wants to comment on
>> the flag choice.  I'll send another reply once I merge the patch.
>
> OK, there is certainly no hurry for merging this. Thanks!
> --
> Michal Hocko
> SUSE Labs

Merged into selinux/next with this patch description, and your
sign-off (I had to munge the description a bit based on the thread).
Are you okay with this, especially your sign-off?

  commit 476accbe2f6ef69caeebe99f52a286e12ac35aee
  Author: Michal Hocko <mhocko@kernel.org>
  Date:   Thu Aug 3 10:11:52 2017 +0200

   selinux: use GFP_NOWAIT in the AVC kmem_caches

   There is a strange __GFP_NOMEMALLOC usage pattern in SELinux,
   specifically GFP_ATOMIC | __GFP_NOMEMALLOC which doesn't make much
   sense.  GFP_ATOMIC on its own allows to access memory reserves while
   __GFP_NOMEMALLOC dictates we cannot use memory reserves.  Replace this
   with the much more sane GFP_NOWAIT in the AVC code as we can tolerate
   memory allocation failures in that code.

   Signed-off-by: Michal Hocko <mhocko@kernel.org>
   Acked-by: Mel Gorman <mgorman@suse.de>
   Signed-off-by: Paul Moore <paul@paul-moore.com>

-- 
paul moore
www.paul-moore.com

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 04-08-17 13:12:04, Paul Moore wrote:
>> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> [...]
>> > Btw. Should I resend the patch or somebody will take it from this email
>> > thread?
>>
>> No, unless your mailer mangled the patch I should be able to pull it
>> from this thread.  However, I'm probably going to let this sit until
>> early next week on the odd chance that anyone else wants to comment on
>> the flag choice.  I'll send another reply once I merge the patch.
>
> OK, there is certainly no hurry for merging this. Thanks!
> --
> Michal Hocko
> SUSE Labs

Merged into selinux/next with this patch description, and your
sign-off (I had to munge the description a bit based on the thread).
Are you okay with this, especially your sign-off?

  commit 476accbe2f6ef69caeebe99f52a286e12ac35aee
  Author: Michal Hocko <mhocko@kernel.org>
  Date:   Thu Aug 3 10:11:52 2017 +0200

   selinux: use GFP_NOWAIT in the AVC kmem_caches

   There is a strange __GFP_NOMEMALLOC usage pattern in SELinux,
   specifically GFP_ATOMIC | __GFP_NOMEMALLOC which doesn't make much
   sense.  GFP_ATOMIC on its own allows to access memory reserves while
   __GFP_NOMEMALLOC dictates we cannot use memory reserves.  Replace this
   with the much more sane GFP_NOWAIT in the AVC code as we can tolerate
   memory allocation failures in that code.

   Signed-off-by: Michal Hocko <mhocko@kernel.org>
   Acked-by: Mel Gorman <mgorman@suse.de>
   Signed-off-by: Paul Moore <paul@paul-moore.com>

-- 
paul moore
www.paul-moore.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Tue 08-08-17 09:34:15, Paul Moore wrote:
> On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 04-08-17 13:12:04, Paul Moore wrote:
> >> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > [...]
> >> > Btw. Should I resend the patch or somebody will take it from this email
> >> > thread?
> >>
> >> No, unless your mailer mangled the patch I should be able to pull it
> >> from this thread.  However, I'm probably going to let this sit until
> >> early next week on the odd chance that anyone else wants to comment on
> >> the flag choice.  I'll send another reply once I merge the patch.
> >
> > OK, there is certainly no hurry for merging this. Thanks!
> > --
> > Michal Hocko
> > SUSE Labs
> 
> Merged into selinux/next with this patch description, and your
> sign-off (I had to munge the description a bit based on the thread).
> Are you okay with this, especially your sign-off?

Yes. Thanks!

> 
>   commit 476accbe2f6ef69caeebe99f52a286e12ac35aee
>   Author: Michal Hocko <mhocko@kernel.org>
>   Date:   Thu Aug 3 10:11:52 2017 +0200
> 
>    selinux: use GFP_NOWAIT in the AVC kmem_caches
> 
>    There is a strange __GFP_NOMEMALLOC usage pattern in SELinux,
>    specifically GFP_ATOMIC | __GFP_NOMEMALLOC which doesn't make much
>    sense.  GFP_ATOMIC on its own allows to access memory reserves while
>    __GFP_NOMEMALLOC dictates we cannot use memory reserves.  Replace this
>    with the much more sane GFP_NOWAIT in the AVC code as we can tolerate
>    memory allocation failures in that code.
> 
>    Signed-off-by: Michal Hocko <mhocko@kernel.org>
>    Acked-by: Mel Gorman <mgorman@suse.de>
>    Signed-off-by: Paul Moore <paul@paul-moore.com>
> 
> -- 
> paul moore
> www.paul-moore.com

-- 
Michal Hocko
SUSE Labs

Re: suspicious __GFP_NOMEMALLOC in selinux

[Michal Hocko]

On Tue 08-08-17 09:34:15, Paul Moore wrote:
> On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Fri 04-08-17 13:12:04, Paul Moore wrote:
> >> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > [...]
> >> > Btw. Should I resend the patch or somebody will take it from this email
> >> > thread?
> >>
> >> No, unless your mailer mangled the patch I should be able to pull it
> >> from this thread.  However, I'm probably going to let this sit until
> >> early next week on the odd chance that anyone else wants to comment on
> >> the flag choice.  I'll send another reply once I merge the patch.
> >
> > OK, there is certainly no hurry for merging this. Thanks!
> > --
> > Michal Hocko
> > SUSE Labs
> 
> Merged into selinux/next with this patch description, and your
> sign-off (I had to munge the description a bit based on the thread).
> Are you okay with this, especially your sign-off?

Yes. Thanks!

> 
>   commit 476accbe2f6ef69caeebe99f52a286e12ac35aee
>   Author: Michal Hocko <mhocko@kernel.org>
>   Date:   Thu Aug 3 10:11:52 2017 +0200
> 
>    selinux: use GFP_NOWAIT in the AVC kmem_caches
> 
>    There is a strange __GFP_NOMEMALLOC usage pattern in SELinux,
>    specifically GFP_ATOMIC | __GFP_NOMEMALLOC which doesn't make much
>    sense.  GFP_ATOMIC on its own allows to access memory reserves while
>    __GFP_NOMEMALLOC dictates we cannot use memory reserves.  Replace this
>    with the much more sane GFP_NOWAIT in the AVC code as we can tolerate
>    memory allocation failures in that code.
> 
>    Signed-off-by: Michal Hocko <mhocko@kernel.org>
>    Acked-by: Mel Gorman <mgorman@suse.de>
>    Signed-off-by: Paul Moore <paul@paul-moore.com>
> 
> -- 
> paul moore
> www.paul-moore.com

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Thu, Aug 10, 2017 at 3:02 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 08-08-17 09:34:15, Paul Moore wrote:
>> On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Fri 04-08-17 13:12:04, Paul Moore wrote:
>> >> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > [...]
>> >> > Btw. Should I resend the patch or somebody will take it from this email
>> >> > thread?
>> >>
>> >> No, unless your mailer mangled the patch I should be able to pull it
>> >> from this thread.  However, I'm probably going to let this sit until
>> >> early next week on the odd chance that anyone else wants to comment on
>> >> the flag choice.  I'll send another reply once I merge the patch.
>> >
>> > OK, there is certainly no hurry for merging this. Thanks!
>> > --
>> > Michal Hocko
>> > SUSE Labs
>>
>> Merged into selinux/next with this patch description, and your
>> sign-off (I had to munge the description a bit based on the thread).
>> Are you okay with this, especially your sign-off?
>
> Yes. Thanks!

Great, thanks for the confirmation.  I'll send this up during the next
merge window.

-- 
paul moore
www.paul-moore.com

Re: suspicious __GFP_NOMEMALLOC in selinux

[Paul Moore]

On Thu, Aug 10, 2017 at 3:02 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 08-08-17 09:34:15, Paul Moore wrote:
>> On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Fri 04-08-17 13:12:04, Paul Moore wrote:
>> >> On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> > [...]
>> >> > Btw. Should I resend the patch or somebody will take it from this email
>> >> > thread?
>> >>
>> >> No, unless your mailer mangled the patch I should be able to pull it
>> >> from this thread.  However, I'm probably going to let this sit until
>> >> early next week on the odd chance that anyone else wants to comment on
>> >> the flag choice.  I'll send another reply once I merge the patch.
>> >
>> > OK, there is certainly no hurry for merging this. Thanks!
>> > --
>> > Michal Hocko
>> > SUSE Labs
>>
>> Merged into selinux/next with this patch description, and your
>> sign-off (I had to munge the description a bit based on the thread).
>> Are you okay with this, especially your sign-off?
>
> Yes. Thanks!

Great, thanks for the confirmation.  I'll send this up during the next
merge window.

-- 
paul moore
www.paul-moore.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>