Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
On Thu 13-09-18 09:12:04, peter enderborg wrote: > On 09/13/2018 08:26 AM, Tetsuo Handa wrote: > > On 2018/09/13 12:02, Paul Moore wrote: > >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa > >> wrote: > >>> syzbot is hitting warning at str_read() [1] because len parameter can > >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for > >>> this case. > >>> > >>> [1] > >>> https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0 > >>> > >>> Signed-off-by: Tetsuo Handa > >>> Reported-by: syzbot > >>> > >>> --- > >>> security/selinux/ss/policydb.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c > >>> b/security/selinux/ss/policydb.c > >>> index e9394e7..f4eadd3 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void > >>> *fp, u32 len) > >>> if ((len == 0) || (len == (u32)-1)) > >>> return -EINVAL; > >>> > >>> - str = kmalloc(len + 1, flags); > >>> + str = kmalloc(len + 1, flags | __GFP_NOWARN); > >>> if (!str) > >>> return -ENOMEM; > >> Thanks for the patch. > >> > >> My eyes are starting to glaze over a bit chasing down all of the > >> different kmalloc() code paths trying to ensure that this always does > >> the right thing based on size of the allocation and the different slab > >> allocators ... are we sure that this will always return NULL when (len > >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator > >> configurations? > >> > > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return > > ZERO_SIZE_PTR) due to (len == (u32)-1) check above. > > > > The only concern would be whether you want allocation failure messages. > > I assumed you don't need it because we are returning -ENOMEM to the caller. > > > Would it not be better with > > char *str; > > if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE)) > return -EINVAL; > > str = kmalloc(len + 1, flags); > if (!str) > return -ENOMEM; I strongly suspect that you want kvmalloc rather than kmalloc here. The larger the request the more likely is the allocation to fail. I am not familiar with the code but I assume this is a root only interface so we don't have to worry about nasty users scenario. -- Michal Hocko SUSE Labs ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: suspicious __GFP_NOMEMALLOC in selinux
On Tue 08-08-17 09:34:15, Paul Moore wrote: > On Mon, Aug 7, 2017 at 2:58 AM, Michal Hocko <mho...@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 <mho...@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 <mho...@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 <mho...@kernel.org> >Acked-by: Mel Gorman <mgor...@suse.de> >Signed-off-by: Paul Moore <p...@paul-moore.com> > > -- > paul moore > www.paul-moore.com -- Michal Hocko SUSE Labs
Re: suspicious __GFP_NOMEMALLOC in selinux
On Fri 04-08-17 13:12:04, Paul Moore wrote: > On Fri, Aug 4, 2017 at 3:56 AM, Michal Hocko <mho...@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
On Thu 03-08-17 14:17:26, Paul Moore wrote: > On Thu, Aug 3, 2017 at 7:05 AM, Michal Hocko <mho...@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
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 <mho...@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
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 <mho...@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
[CC Mel]
On Wed 02-08-17 17:45:56, Paul Moore wrote:
> On Wed, Aug 2, 2017 at 6:50 AM, Michal Hocko <mho...@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 <mho...@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 <mho...@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 = _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 =
