* Re: Can kfree() sleep at runtime?
[not found] <30ecafd7-ed61-907b-f924-77fc37dcc753@gmail.com>
@ 2018-05-31 14:08 ` Matthew Wilcox
2018-05-31 14:12 ` Christopher Lameter
2018-06-01 1:12 ` Jia-Ju Bai
2018-05-31 14:09 ` Christopher Lameter
1 sibling, 2 replies; 9+ messages in thread
From: Matthew Wilcox @ 2018-05-31 14:08 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
Linux Kernel Mailing List
On Thu, May 31, 2018 at 09:10:07PM +0800, Jia-Ju Bai wrote:
> I write a static analysis tool (DSAC), and it finds that kfree() can sleep.
>
> Here is the call path for kfree().
> Please look at it *from the bottom up*.
>
> [FUNC] alloc_pages(GFP_KERNEL)
> arch/x86/mm/pageattr.c, 756: alloc_pages in split_large_page
> arch/x86/mm/pageattr.c, 1283: split_large_page in __change_page_attr
Here's your bug. Coming from kfree(), we can't end up in the
split_large_page() path. __change_page_attr may be called in several
different circumstances in which it would have to split a large page,
but the path from kfree() is not one of them.
I think the path from kfree() will lead to the 'level == PG_LEVEL_4K'
path, but I'm not really familiar with this x86 code.
> arch/x86/mm/pageattr.c, 1391: __change_page_attr in
> __change_page_attr_set_clr
> arch/x86/mm/pageattr.c, 2014: __change_page_attr_set_clr in __set_pages_np
> arch/x86/mm/pageattr.c, 2034: __set_pages_np in __kernel_map_pages
> ./include/linux/mm.h, 2488: __kernel_map_pages in kernel_map_pages
> mm/page_alloc.c, 1074: kernel_map_pages in free_pages_prepare
> mm/page_alloc.c, 1264: free_pages_prepare in __free_pages_ok
> mm/page_alloc.c, 4312: __free_pages_ok in __free_pages
> mm/slub.c, 3914: __free_pages in kfree
>
> I always have an impression that kfree() never sleeps, so I feel confused
> here.
> So could someone please help me to find the mistake?
> Thanks in advance :)
>
> Best wishes,
> Jia-Ju Bai
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
[not found] <30ecafd7-ed61-907b-f924-77fc37dcc753@gmail.com>
2018-05-31 14:08 ` Can kfree() sleep at runtime? Matthew Wilcox
@ 2018-05-31 14:09 ` Christopher Lameter
2018-06-01 1:18 ` Jia-Ju Bai
1 sibling, 1 reply; 9+ messages in thread
From: Christopher Lameter @ 2018-05-31 14:09 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: Pekka Enberg, David Rientjes, Joonsoo Kim, akpm, linux-mm,
Mel Gorman, Linux Kernel Mailing List
On Thu, 31 May 2018, Jia-Ju Bai wrote:
> I write a static analysis tool (DSAC), and it finds that kfree() can sleep.
That should not happen.
> Here is the call path for kfree().
> Please look at it *from the bottom up*.
>
> [FUNC] alloc_pages(GFP_KERNEL)
> arch/x86/mm/pageattr.c, 756: alloc_pages in split_large_page
> arch/x86/mm/pageattr.c, 1283: split_large_page in __change_page_attr
> arch/x86/mm/pageattr.c, 1391: __change_page_attr in __change_page_attr_set_clr
> arch/x86/mm/pageattr.c, 2014: __change_page_attr_set_clr in __set_pages_np
> arch/x86/mm/pageattr.c, 2034: __set_pages_np in __kernel_map_pages
> ./include/linux/mm.h, 2488: __kernel_map_pages in kernel_map_pages
> mm/page_alloc.c, 1074: kernel_map_pages in free_pages_prepare
mapping pages in the page allocator can cause allocations?? How did that
get in there?
> mm/page_alloc.c, 1264: free_pages_prepare in __free_pages_ok
> mm/page_alloc.c, 4312: __free_pages_ok in __free_pages
> mm/slub.c, 3914: __free_pages in kfree
>
> I always have an impression that kfree() never sleeps, so I feel confused
> here.
Correct.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-05-31 14:08 ` Can kfree() sleep at runtime? Matthew Wilcox
@ 2018-05-31 14:12 ` Christopher Lameter
2018-05-31 14:14 ` Matthew Wilcox
2018-06-01 1:12 ` Jia-Ju Bai
1 sibling, 1 reply; 9+ messages in thread
From: Christopher Lameter @ 2018-05-31 14:12 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jia-Ju Bai, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
Linux Kernel Mailing List
On Thu, 31 May 2018, Matthew Wilcox wrote:
> On Thu, May 31, 2018 at 09:10:07PM +0800, Jia-Ju Bai wrote:
> > I write a static analysis tool (DSAC), and it finds that kfree() can sleep.
> >
> > Here is the call path for kfree().
> > Please look at it *from the bottom up*.
> >
> > [FUNC] alloc_pages(GFP_KERNEL)
> > arch/x86/mm/pageattr.c, 756: alloc_pages in split_large_page
> > arch/x86/mm/pageattr.c, 1283: split_large_page in __change_page_attr
>
> Here's your bug. Coming from kfree(), we can't end up in the
> split_large_page() path. __change_page_attr may be called in several
> different circumstances in which it would have to split a large page,
> but the path from kfree() is not one of them.
Freeing a page in the page allocator also was traditionally not sleeping.
That has changed?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-05-31 14:12 ` Christopher Lameter
@ 2018-05-31 14:14 ` Matthew Wilcox
2018-05-31 14:30 ` Christopher Lameter
0 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2018-05-31 14:14 UTC (permalink / raw)
To: Christopher Lameter
Cc: Jia-Ju Bai, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
Linux Kernel Mailing List
On Thu, May 31, 2018 at 02:12:00PM +0000, Christopher Lameter wrote:
> On Thu, 31 May 2018, Matthew Wilcox wrote:
>
> > On Thu, May 31, 2018 at 09:10:07PM +0800, Jia-Ju Bai wrote:
> > > I write a static analysis tool (DSAC), and it finds that kfree() can sleep.
> > >
> > > Here is the call path for kfree().
> > > Please look at it *from the bottom up*.
> > >
> > > [FUNC] alloc_pages(GFP_KERNEL)
> > > arch/x86/mm/pageattr.c, 756: alloc_pages in split_large_page
> > > arch/x86/mm/pageattr.c, 1283: split_large_page in __change_page_attr
> >
> > Here's your bug. Coming from kfree(), we can't end up in the
> > split_large_page() path. __change_page_attr may be called in several
> > different circumstances in which it would have to split a large page,
> > but the path from kfree() is not one of them.
>
> Freeing a page in the page allocator also was traditionally not sleeping.
> That has changed?
No. "Your bug" being "The bug in your static analysis tool". It probably
isn't following the data flow correctly (or deeply enough).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-05-31 14:14 ` Matthew Wilcox
@ 2018-05-31 14:30 ` Christopher Lameter
2018-06-01 1:22 ` Jia-Ju Bai
0 siblings, 1 reply; 9+ messages in thread
From: Christopher Lameter @ 2018-05-31 14:30 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jia-Ju Bai, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
Linux Kernel Mailing List
On Thu, 31 May 2018, Matthew Wilcox wrote:
> > Freeing a page in the page allocator also was traditionally not sleeping.
> > That has changed?
>
> No. "Your bug" being "The bug in your static analysis tool". It probably
> isn't following the data flow correctly (or deeply enough).
Well ok this is not going to trigger for kfree(), this is x86 specific and
requires CONFIG_DEBUG_PAGEALLOC and a free of a page in a huge page.
Ok that is a very contorted situation but how would a static checker deal
with that?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-05-31 14:08 ` Can kfree() sleep at runtime? Matthew Wilcox
2018-05-31 14:12 ` Christopher Lameter
@ 2018-06-01 1:12 ` Jia-Ju Bai
1 sibling, 0 replies; 9+ messages in thread
From: Jia-Ju Bai @ 2018-06-01 1:12 UTC (permalink / raw)
To: Matthew Wilcox
Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
Linux Kernel Mailing List
On 2018/5/31 22:08, Matthew Wilcox wrote:
> On Thu, May 31, 2018 at 09:10:07PM +0800, Jia-Ju Bai wrote:
>> I write a static analysis tool (DSAC), and it finds that kfree() can sleep.
>>
>> Here is the call path for kfree().
>> Please look at it *from the bottom up*.
>>
>> [FUNC] alloc_pages(GFP_KERNEL)
>> arch/x86/mm/pageattr.c, 756: alloc_pages in split_large_page
>> arch/x86/mm/pageattr.c, 1283: split_large_page in __change_page_attr
> Here's your bug. Coming from kfree(), we can't end up in the
> split_large_page() path. __change_page_attr may be called in several
> different circumstances in which it would have to split a large page,
> but the path from kfree() is not one of them.
>
> I think the path from kfree() will lead to the 'level == PG_LEVEL_4K'
> path, but I'm not really familiar with this x86 code.
Thanks for reply :)
But from the code in my call path, I cannot find why kfree() will only lead to the 'level == PG_LEVEL_4K' path.
Could you please explain it in more detail?
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-05-31 14:09 ` Christopher Lameter
@ 2018-06-01 1:18 ` Jia-Ju Bai
0 siblings, 0 replies; 9+ messages in thread
From: Jia-Ju Bai @ 2018-06-01 1:18 UTC (permalink / raw)
To: Christopher Lameter
Cc: Pekka Enberg, David Rientjes, Joonsoo Kim, akpm, linux-mm,
Mel Gorman, Linux Kernel Mailing List
On 2018/5/31 22:09, Christopher Lameter wrote:
> On Thu, 31 May 2018, Jia-Ju Bai wrote:
>
>> I write a static analysis tool (DSAC), and it finds that kfree() can sleep.
> That should not happen.
>
>> Here is the call path for kfree().
>> Please look at it *from the bottom up*.
>>
>> [FUNC] alloc_pages(GFP_KERNEL)
>> arch/x86/mm/pageattr.c, 756: alloc_pages in split_large_page
>> arch/x86/mm/pageattr.c, 1283: split_large_page in __change_page_attr
>> arch/x86/mm/pageattr.c, 1391: __change_page_attr in __change_page_attr_set_clr
>> arch/x86/mm/pageattr.c, 2014: __change_page_attr_set_clr in __set_pages_np
>> arch/x86/mm/pageattr.c, 2034: __set_pages_np in __kernel_map_pages
>> ./include/linux/mm.h, 2488: __kernel_map_pages in kernel_map_pages
>> mm/page_alloc.c, 1074: kernel_map_pages in free_pages_prepare
> mapping pages in the page allocator can cause allocations?? How did that
> get in there?
Thanks for reply :)
I am also confused about it.
I get in here according to the definition of free_pages_prepare():
1022. static bool free_pages_prepare(...) {
...
1072. arch_free_page(page, order);
1073. kernel_poison_pages(page, 1 << order, 0);
1074. kernel_map_pages(page, 1 << order, 0); // *Here*
1075. kasan_free_pages(page, order);
1076.
1077. return true;
1078. }
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-05-31 14:30 ` Christopher Lameter
@ 2018-06-01 1:22 ` Jia-Ju Bai
2018-06-01 1:34 ` Nadav Amit
0 siblings, 1 reply; 9+ messages in thread
From: Jia-Ju Bai @ 2018-06-01 1:22 UTC (permalink / raw)
To: Christopher Lameter, Matthew Wilcox
Cc: penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
Linux Kernel Mailing List
On 2018/5/31 22:30, Christopher Lameter wrote:
> On Thu, 31 May 2018, Matthew Wilcox wrote:
>
>>> Freeing a page in the page allocator also was traditionally not sleeping.
>>> That has changed?
>> No. "Your bug" being "The bug in your static analysis tool". It probably
>> isn't following the data flow correctly (or deeply enough).
> Well ok this is not going to trigger for kfree(), this is x86 specific and
> requires CONFIG_DEBUG_PAGEALLOC and a free of a page in a huge page.
>
> Ok that is a very contorted situation but how would a static checker deal
> with that?
I admit that my tool does not follow the data flow well, and I need to
improve it.
In this case of kfree(), I want know how the data flow leads to my mistake.
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Can kfree() sleep at runtime?
2018-06-01 1:22 ` Jia-Ju Bai
@ 2018-06-01 1:34 ` Nadav Amit
0 siblings, 0 replies; 9+ messages in thread
From: Nadav Amit @ 2018-06-01 1:34 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: Christopher Lameter, Matthew Wilcox, penberg, rientjes,
iamjoonsoo.kim, Andrew Morton, linux-mm,
Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]
Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>
>
> On 2018/5/31 22:30, Christopher Lameter wrote:
>> On Thu, 31 May 2018, Matthew Wilcox wrote:
>>
>>>> Freeing a page in the page allocator also was traditionally not sleeping.
>>>> That has changed?
>>> No. "Your bug" being "The bug in your static analysis tool". It probably
>>> isn't following the data flow correctly (or deeply enough).
>> Well ok this is not going to trigger for kfree(), this is x86 specific and
>> requires CONFIG_DEBUG_PAGEALLOC and a free of a page in a huge page.
>>
>> Ok that is a very contorted situation but how would a static checker deal
>> with that?
>
> I admit that my tool does not follow the data flow well, and I need to improve it.
> In this case of kfree(), I want know how the data flow leads to my mistake.
Note that is only enabled in debug mode:
static inline void
kernel_map_pages(struct page *page, int numpages, int enable)
{
if (!debug_pagealloc_enabled())
return;
__kernel_map_pages(page, numpages, enable);
}
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-06-01 1:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <30ecafd7-ed61-907b-f924-77fc37dcc753@gmail.com>
2018-05-31 14:08 ` Can kfree() sleep at runtime? Matthew Wilcox
2018-05-31 14:12 ` Christopher Lameter
2018-05-31 14:14 ` Matthew Wilcox
2018-05-31 14:30 ` Christopher Lameter
2018-06-01 1:22 ` Jia-Ju Bai
2018-06-01 1:34 ` Nadav Amit
2018-06-01 1:12 ` Jia-Ju Bai
2018-05-31 14:09 ` Christopher Lameter
2018-06-01 1:18 ` Jia-Ju Bai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).