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
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?
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).
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?
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
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
Jia-Ju Bai <[email protected]> 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);
}