[RFC] vmalloc: add warning in __vmalloc

[RFC] vmalloc: add warning in __vmalloc

[Minchan Kim]

Now there are several places to use __vmalloc with GFP_ATOMIC,
GFP_NOIO, GFP_NOFS but unfortunately __vmalloc calls map_vm_area
which calls alloc_pages with GFP_KERNEL to allocate page tables.
It means it's possible to happen deadlock.
I don't know why it doesn't have reported until now.

Firstly, I tried passing gfp_t to lower functions to support __vmalloc
with such flags but other mm guys don't want and decided that
all of caller should be fixed.

http://marc.info/?l=linux-kernel&m=133517143616544&w=2

To begin with, let's listen other's opinion whether they can fix it
by other approach without calling __vmalloc with such flags.

So this patch adds warning to detect and to be fixed hopely.
I Cced related maintainers.
If I miss someone, please Cced them.

side-note:
  I added WARN_ON instead of WARN_ONCE to detect all of callers
  and each WARN_ON for each flag to detect to use any flag easily.
  After we fix all of caller or reduce such caller, we can merge
  a warning with WARN_ONCE.

Cc: Neil Brown <neilb@suse.de>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: James Morris <jmorris@namei.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Sage Weil <sage@newdream.net>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/vmalloc.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 94dff88..36beccb 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1700,6 +1700,15 @@ static void *__vmalloc_node(unsigned long size, unsigned long align,
 			    gfp_t gfp_mask, pgprot_t prot,
 			    int node, void *caller)
 {
+	/*
+	 * This function calls map_vm_area so that it allocates
+	 * page table with GFP_KERNEL so caller should avoid using
+	 * GFP_NOIO, GFP_NOFS and !__GFP_WAIT.
+	 */
+	WARN_ON(!(gfp_mask & __GFP_WAIT));
+	WARN_ON(!(gfp_mask & __GFP_IO));
+	WARN_ON(!(gfp_mask & __GFP_FS));
+
 	return __vmalloc_node_range(size, align, VMALLOC_START, VMALLOC_END,
 				gfp_mask, prot, node, caller);
 }
-- 
1.7.9.5

Re: [RFC] vmalloc: add warning in __vmalloc

[Steven Whitehouse]

Hi,

On Fri, 2012-04-27 at 17:42 +0900, Minchan Kim wrote:
> Now there are several places to use __vmalloc with GFP_ATOMIC,
> GFP_NOIO, GFP_NOFS but unfortunately __vmalloc calls map_vm_area
> which calls alloc_pages with GFP_KERNEL to allocate page tables.
> It means it's possible to happen deadlock.
> I don't know why it doesn't have reported until now.
> 
> Firstly, I tried passing gfp_t to lower functions to support __vmalloc
> with such flags but other mm guys don't want and decided that
> all of caller should be fixed.
> 
> http://marc.info/?l=linux-kernel&m=133517143616544&w=2
> 
> To begin with, let's listen other's opinion whether they can fix it
> by other approach without calling __vmalloc with such flags.
> 
> So this patch adds warning to detect and to be fixed hopely.
> I Cced related maintainers.
> If I miss someone, please Cced them.
> 
That seems ok to me. GFS2 only uses it as a back up in case the kmalloc
call fails, and I suspect that we could easily eliminate it entirely
since I doubt that it does actually ever fail in reality. If it were to
fail then that is handled correctly anyway,

Steve.

> side-note:
>   I added WARN_ON instead of WARN_ONCE to detect all of callers
>   and each WARN_ON for each flag to detect to use any flag easily.
>   After we fix all of caller or reduce such caller, we can merge
>   a warning with WARN_ONCE.
> 
> Cc: Neil Brown <neilb@suse.de>
> Cc: Artem Bityutskiy <dedekind1@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Steven Whitehouse <swhiteho@redhat.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: James Morris <jmorris@namei.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Sage Weil <sage@newdream.net>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/vmalloc.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 94dff88..36beccb 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1700,6 +1700,15 @@ static void *__vmalloc_node(unsigned long size, unsigned long align,
>  			    gfp_t gfp_mask, pgprot_t prot,
>  			    int node, void *caller)
>  {
> +	/*
> +	 * This function calls map_vm_area so that it allocates
> +	 * page table with GFP_KERNEL so caller should avoid using
> +	 * GFP_NOIO, GFP_NOFS and !__GFP_WAIT.
> +	 */
> +	WARN_ON(!(gfp_mask & __GFP_WAIT));
> +	WARN_ON(!(gfp_mask & __GFP_IO));
> +	WARN_ON(!(gfp_mask & __GFP_FS));
> +
>  	return __vmalloc_node_range(size, align, VMALLOC_START, VMALLOC_END,
>  				gfp_mask, prot, node, caller);
>  }


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] vmalloc: add warning in __vmalloc

[David Rientjes]

On Fri, 27 Apr 2012, Minchan Kim wrote:

> Now there are several places to use __vmalloc with GFP_ATOMIC,
> GFP_NOIO, GFP_NOFS but unfortunately __vmalloc calls map_vm_area
> which calls alloc_pages with GFP_KERNEL to allocate page tables.
> It means it's possible to happen deadlock.
> I don't know why it doesn't have reported until now.
> 
> Firstly, I tried passing gfp_t to lower functions to support __vmalloc
> with such flags but other mm guys don't want and decided that
> all of caller should be fixed.
> 
> http://marc.info/?l=linux-kernel&m=133517143616544&w=2
> 
> To begin with, let's listen other's opinion whether they can fix it
> by other approach without calling __vmalloc with such flags.
> 
> So this patch adds warning to detect and to be fixed hopely.
> I Cced related maintainers.
> If I miss someone, please Cced them.
> 
> side-note:
>   I added WARN_ON instead of WARN_ONCE to detect all of callers
>   and each WARN_ON for each flag to detect to use any flag easily.
>   After we fix all of caller or reduce such caller, we can merge
>   a warning with WARN_ONCE.
> 

I disagree with this approach since it's going to violently spam an 
innocent kernel user's log with no ratelimiting and for a situation that 
actually may not be problematic.

Passing any of these bits (the difference between GFP_KERNEL and 
GFP_ATOMIC) only means anything when we're going to do reclaim.  And I'm 
suspecting we would have seen problems with this already since 
pte_alloc_kernel() does __GFP_REPEAT on most architectures meaning that it 
will loop infinitely in the page allocator until at least one page is 
freed (since its an order-0 allocation) which would hardly ever happen if 
__GFP_FS or __GFP_IO actually meant something in this context.

In other words, we would already have seen these deadlocks and it would 
have been diagnosed as a vmalloc(GFP_ATOMIC) problem.  Where are those bug 
reports?

At best, you'd need _some_ sort of ratelimiting like a static variable and 
only allowing 100 WARN_ON()s which could output dozens of lines for each 
call to vmalloc().

But the page allocator already has a might_sleep_if(gfp_mask & GFP_WAIT) 
which will dump the stack for CONFIG_DEBUG_ATOMIC_SLEEP.  So for this 
effect, just enable that config option and check your kernel log.

So I'm afraid this is complete overkill for something that we can't prove 
is a problem in the first place and will potentially fill the kernel logs 
for warnings where the allocation succeeds immediately.  If you want the 
bug reports, ask people to enable CONFIG_DEBUG_ATOMIC_SLEEP.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] vmalloc: add warning in __vmalloc

[Nick Piggin]

On 27 April 2012 20:36, David Rientjes <rientjes@google.com> wrote:
> On Fri, 27 Apr 2012, Minchan Kim wrote:
>
>> Now there are several places to use __vmalloc with GFP_ATOMIC,
>> GFP_NOIO, GFP_NOFS but unfortunately __vmalloc calls map_vm_area
>> which calls alloc_pages with GFP_KERNEL to allocate page tables.
>> It means it's possible to happen deadlock.
>> I don't know why it doesn't have reported until now.
>>
>> Firstly, I tried passing gfp_t to lower functions to support __vmalloc
>> with such flags but other mm guys don't want and decided that
>> all of caller should be fixed.
>>
>> http://marc.info/?l=linux-kernel&m=133517143616544&w=2
>>
>> To begin with, let's listen other's opinion whether they can fix it
>> by other approach without calling __vmalloc with such flags.
>>
>> So this patch adds warning to detect and to be fixed hopely.
>> I Cced related maintainers.
>> If I miss someone, please Cced them.
>>
>> side-note:
>>   I added WARN_ON instead of WARN_ONCE to detect all of callers
>>   and each WARN_ON for each flag to detect to use any flag easily.
>>   After we fix all of caller or reduce such caller, we can merge
>>   a warning with WARN_ONCE.
>>
>
> I disagree with this approach since it's going to violently spam an
> innocent kernel user's log with no ratelimiting and for a situation that
> actually may not be problematic.

With WARN_ON_ONCE, it should be good.

>
> Passing any of these bits (the difference between GFP_KERNEL and
> GFP_ATOMIC) only means anything when we're going to do reclaim.  And I'm
> suspecting we would have seen problems with this already since
> pte_alloc_kernel() does __GFP_REPEAT on most architectures meaning that it
> will loop infinitely in the page allocator until at least one page is
> freed (since its an order-0 allocation) which would hardly ever happen if
> __GFP_FS or __GFP_IO actually meant something in this context.
>
> In other words, we would already have seen these deadlocks and it would
> have been diagnosed as a vmalloc(GFP_ATOMIC) problem.  Where are those bug
> reports?

That's not sound logic to disprove a bug.

I think simply most callers are permissive and don't mask out flags.
But for example a filesystem holding an fs lock and then doing
vmalloc(GFP_NOFS) can certainly deadlock.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] vmalloc: add warning in __vmalloc

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2020 bytes --]

On Tue, 1 May 2012, Nick Piggin wrote:

> > I disagree with this approach since it's going to violently spam an
> > innocent kernel user's log with no ratelimiting and for a situation that
> > actually may not be problematic.
> 
> With WARN_ON_ONCE, it should be good.
> 

To catch a single instance of this per-boot, sure.  I've never seen us add 
WARN_ON_ONCE()'s where we have concrete examples of kernel code that will 
trigger it, though.  Not sure why spamming the kernel log and getting 
users to think something is wrong and report the bug when it's possible to 
audit the code and make a report to the subsystem maintainer.  Perhaps 
adding WARN_ON_ONCE()'s is just easier and then walk away from it?

> > Passing any of these bits (the difference between GFP_KERNEL and
> > GFP_ATOMIC) only means anything when we're going to do reclaim. A And I'm
> > suspecting we would have seen problems with this already since
> > pte_alloc_kernel() does __GFP_REPEAT on most architectures meaning that it
> > will loop infinitely in the page allocator until at least one page is
> > freed (since its an order-0 allocation) which would hardly ever happen if
> > __GFP_FS or __GFP_IO actually meant something in this context.
> >
> > In other words, we would already have seen these deadlocks and it would
> > have been diagnosed as a vmalloc(GFP_ATOMIC) problem. A Where are those bug
> > reports?
> 
> That's not sound logic to disprove a bug.
> 
> I think simply most callers are permissive and don't mask out flags.
> But for example a filesystem holding an fs lock and then doing
> vmalloc(GFP_NOFS) can certainly deadlock.
> 

I'm not disproving a bug, I'm asking for an example of how this problem 
has caused pain before and it has been the result of calling 
vmalloc(GFP_NOFS).  I agree we should certainly fix those callers, but it 
seems like adding the WARN_ON_ONCE()'s is certainly going to cause pain in 
tons of bug reports where there's no actual problem that couldn't have 
been found by auditing the code.

Re: [RFC] vmalloc: add warning in __vmalloc

[Nick Piggin]

On 2 May 2012 06:22, David Rientjes <rientjes@google.com> wrote:
> On Tue, 1 May 2012, Nick Piggin wrote:
>
>> > I disagree with this approach since it's going to violently spam an
>> > innocent kernel user's log with no ratelimiting and for a situation that
>> > actually may not be problematic.
>>
>> With WARN_ON_ONCE, it should be good.
>>
>
> To catch a single instance of this per-boot, sure.  I've never seen us add
> WARN_ON_ONCE()'s where we have concrete examples of kernel code that will
> trigger it, though.  Not sure why spamming the kernel log and getting
> users to think something is wrong and report the bug when it's possible to
> audit the code and make a report to the subsystem maintainer.

Because it needs to be an ongoing thing, which is caught as soon as the
developer writes some code, rather than continually audited for and fixed
up after the fact. There is not a good way to enforce this at compile time.

The existing callers do need to be fixed too, of course.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] vmalloc: add warning in __vmalloc

[David Rientjes]

On Wed, 2 May 2012, Nick Piggin wrote:

> Because it needs to be an ongoing thing, which is caught as soon as the
> developer writes some code, rather than continually audited for and fixed
> up after the fact. There is not a good way to enforce this at compile time.
> 
> The existing callers do need to be fixed too, of course.
> 

I'm asking that existing callers be fixed up before such a warning is 
introduced.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] vmalloc: add warning in __vmalloc

[Andrew Morton]

On Fri, 27 Apr 2012 17:42:24 +0900
Minchan Kim <minchan@kernel.org> wrote:

> Now there are several places to use __vmalloc with GFP_ATOMIC,
> GFP_NOIO, GFP_NOFS but unfortunately __vmalloc calls map_vm_area
> which calls alloc_pages with GFP_KERNEL to allocate page tables.
> It means it's possible to happen deadlock.
> I don't know why it doesn't have reported until now.
> 
> Firstly, I tried passing gfp_t to lower functions to support __vmalloc
> with such flags but other mm guys don't want and decided that
> all of caller should be fixed.
> 
> http://marc.info/?l=linux-kernel&m=133517143616544&w=2
> 
> To begin with, let's listen other's opinion whether they can fix it
> by other approach without calling __vmalloc with such flags.
> 
> So this patch adds warning to detect and to be fixed hopely.
> I Cced related maintainers.
> If I miss someone, please Cced them.
> 
> side-note:
>   I added WARN_ON instead of WARN_ONCE to detect all of callers
>   and each WARN_ON for each flag to detect to use any flag easily.
>   After we fix all of caller or reduce such caller, we can merge
>   a warning with WARN_ONCE.

Just WARN_ONCE, please.  If that exposes some sort of calamity then we
can reconsider.

> 
> ...
>
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1700,6 +1700,15 @@ static void *__vmalloc_node(unsigned long size, unsigned long align,
>  			    gfp_t gfp_mask, pgprot_t prot,
>  			    int node, void *caller)
>  {
> +	/*
> +	 * This function calls map_vm_area so that it allocates
> +	 * page table with GFP_KERNEL so caller should avoid using
> +	 * GFP_NOIO, GFP_NOFS and !__GFP_WAIT.
> +	 */
> +	WARN_ON(!(gfp_mask & __GFP_WAIT));
> +	WARN_ON(!(gfp_mask & __GFP_IO));
> +	WARN_ON(!(gfp_mask & __GFP_FS));
> +
>  	return __vmalloc_node_range(size, align, VMALLOC_START, VMALLOC_END,
>  				gfp_mask, prot, node, caller);
>  }

This seems strange.  There are many entry points to this code and the
patch appears to go into a randomly-chosen middle point in the various
call chains and sticks a check in there.  Why was __vmalloc_node()
chosen?  Does this provide full coverage or all entry points?



Also, the patch won't warn in the most problematic cases such as
vmalloc() being called from a __GFP_NOFS context.  Presumably there are
might_sleep() warnings somewhere on the allocation path which will
catch vmalloc() being called from atomic contexts.

I'm not sure what to do about that - we don't have machinery in place
to be able to detect when a GFP_KERNEL allocation is deadlockable. 
Perhaps a lot of hacking on lockdep might get us this - we'd need to
teach lockdep about which locks prohibit FS entry, which locks prevent
IO entry, etc.  And there are secret locks such as ext3/4
journal_start(), and bitlocks and lock_page().  eek.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] vmalloc: add warning in __vmalloc

[Minchan Kim]

On 04/28/2012 07:00 AM, Andrew Morton wrote:

> On Fri, 27 Apr 2012 17:42:24 +0900
> Minchan Kim <minchan@kernel.org> wrote:
> 
>> Now there are several places to use __vmalloc with GFP_ATOMIC,
>> GFP_NOIO, GFP_NOFS but unfortunately __vmalloc calls map_vm_area
>> which calls alloc_pages with GFP_KERNEL to allocate page tables.
>> It means it's possible to happen deadlock.
>> I don't know why it doesn't have reported until now.
>>
>> Firstly, I tried passing gfp_t to lower functions to support __vmalloc
>> with such flags but other mm guys don't want and decided that
>> all of caller should be fixed.
>>
>> http://marc.info/?l=linux-kernel&m=133517143616544&w=2
>>
>> To begin with, let's listen other's opinion whether they can fix it
>> by other approach without calling __vmalloc with such flags.
>>
>> So this patch adds warning to detect and to be fixed hopely.
>> I Cced related maintainers.
>> If I miss someone, please Cced them.
>>
>> side-note:
>>   I added WARN_ON instead of WARN_ONCE to detect all of callers
>>   and each WARN_ON for each flag to detect to use any flag easily.
>>   After we fix all of caller or reduce such caller, we can merge
>>   a warning with WARN_ONCE.
> 
> Just WARN_ONCE, please.  If that exposes some sort of calamity then we
> can reconsider.


NP.

> 
>>
>> ...
>>
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -1700,6 +1700,15 @@ static void *__vmalloc_node(unsigned long size, unsigned long align,
>>  			    gfp_t gfp_mask, pgprot_t prot,
>>  			    int node, void *caller)
>>  {
>> +	/*
>> +	 * This function calls map_vm_area so that it allocates
>> +	 * page table with GFP_KERNEL so caller should avoid using
>> +	 * GFP_NOIO, GFP_NOFS and !__GFP_WAIT.
>> +	 */
>> +	WARN_ON(!(gfp_mask & __GFP_WAIT));
>> +	WARN_ON(!(gfp_mask & __GFP_IO));
>> +	WARN_ON(!(gfp_mask & __GFP_FS));
>> +
>>  	return __vmalloc_node_range(size, align, VMALLOC_START, VMALLOC_END,
>>  				gfp_mask, prot, node, caller);
>>  }
> 
> This seems strange.  There are many entry points to this code and the
> patch appears to go into a randomly-chosen middle point in the various
> call chains and sticks a check in there.  Why was __vmalloc_node()
> chosen?  Does this provide full coverage or all entry points?


I think it covers all of caller with calls __vmalloc with gfp_flags.
Only exception is __vmalloc_node_range which is called by module_alloc
but it surely calls __vmalloc_node_range with GFP_KERNEL so it's no problem now.
If you want to catch potential use of __vmalloc_node_range in future, I can move it
to it. 

> 
> 
> 
> Also, the patch won't warn in the most problematic cases such as
> vmalloc() being called from a __GFP_NOFS context.  Presumably there are


I agree but this patch's goal is just to prevent calling __vmalloc with GFP_ATOMIC, GFP_NOFS and
GFP_NOIO. We should consider vmalloc on __GFP_NOFS context as another problem and maybe
reclaimfs lockdep would be a good start point.

> might_sleep() warnings somewhere on the allocation path which will

> catch vmalloc() being called from atomic contexts.


Yes.

> 
> I'm not sure what to do about that - we don't have machinery in place
> to be able to detect when a GFP_KERNEL allocation is deadlockable. 
> Perhaps a lot of hacking on lockdep might get us this - we'd need to
> teach lockdep about which locks prohibit FS entry, which locks prevent
> IO entry, etc.  And there are secret locks such as ext3/4
> journal_start(), and bitlocks and lock_page().  eek.

> 

> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>