[PATCH resend] slub: Add back check for free nonslab objects

[PATCH resend] slub: Add back check for free nonslab objects

[Kefeng Wang]

After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk
free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE,
which only check with CONFIG_DEBUG_VM enabled, but this config may
impact performance, so it only for debug.

Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
add the ability, which should be needed in any configs to catch the
invalid free, they even could be potential issue, eg, memory corruption,
use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
add dump_page() to help use to debug the issue.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 mm/slub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3095b889fab4..555c2e6ccfca 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object)
 {
 	unsigned int order = compound_order(page);
 
-	VM_BUG_ON_PAGE(!PageCompound(page), page);
+	if (WARN_ON(!PageCompound(page)))
+		dump_page(page, "invalid free nonslab page");
 	kfree_hook(object);
 	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
 	__free_pages(page, order);
-- 
2.26.2

Re: [PATCH resend] slub: Add back check for free nonslab objects

[Matthew Wilcox]

On Mon, Sep 27, 2021 at 10:15:38AM +0800, Kefeng Wang wrote:
> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
> add the ability, which should be needed in any configs to catch the
> invalid free, they even could be potential issue, eg, memory corruption,
> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
> add dump_page() to help use to debug the issue.

Is dump_page() really the best way to catch such a thing?  I would have
thought that printing the address of 'object' would be more helpful.

> @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object)
>  {
>  	unsigned int order = compound_order(page);
>  
> -	VM_BUG_ON_PAGE(!PageCompound(page), page);
> +	if (WARN_ON(!PageCompound(page)))
> +		dump_page(page, "invalid free nonslab page");

Re: [PATCH resend] slub: Add back check for free nonslab objects

[Kefeng Wang]

On 2021/9/27 10:42, Matthew Wilcox wrote:
> On Mon, Sep 27, 2021 at 10:15:38AM +0800, Kefeng Wang wrote:
>> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
>> add the ability, which should be needed in any configs to catch the
>> invalid free, they even could be potential issue, eg, memory corruption,
>> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
>> add dump_page() to help use to debug the issue.
> Is dump_page() really the best way to catch such a thing?  I would have
> thought that printing the address of 'object' would be more helpful.

With no vmcore, dump_page() do help us to find a memory corruption issue.

We could add 'object ' address print too.

>
>> @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object)
>>   {
>>   	unsigned int order = compound_order(page);
>>   
>> -	VM_BUG_ON_PAGE(!PageCompound(page), page);
>> +	if (WARN_ON(!PageCompound(page)))
>> +		dump_page(page, "invalid free nonslab page");
> .
>

Re: [PATCH resend] slub: Add back check for free nonslab objects

[Vlastimil Babka]

On 9/27/21 04:15, Kefeng Wang wrote:
> After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk
> free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE,
> which only check with CONFIG_DEBUG_VM enabled, but this config may
> impact performance, so it only for debug.
> 
> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
> add the ability, which should be needed in any configs to catch the
> invalid free, they even could be potential issue, eg, memory corruption,
> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
> add dump_page() to help use to debug the issue.

There are other situations in SLUB (such as with smaller allocations that
don't go directly to page allocator) where use after free and double-free
are undetected in non-debug configs, and it's expected that anyone debugging
them will enable slub_debug or even DEBUG_VM. Why should this special case
with nonslab pages be different?

> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  mm/slub.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 3095b889fab4..555c2e6ccfca 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object)
>  {
>  	unsigned int order = compound_order(page);
>  
> -	VM_BUG_ON_PAGE(!PageCompound(page), page);
> +	if (WARN_ON(!PageCompound(page)))
> +		dump_page(page, "invalid free nonslab page");
>  	kfree_hook(object);
>  	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
>  	__free_pages(page, order);
> 

Re: [PATCH resend] slub: Add back check for free nonslab objects

[Kefeng Wang]

On 2021/9/27 15:22, Vlastimil Babka wrote:
> On 9/27/21 04:15, Kefeng Wang wrote:
>> After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk
>> free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE,
>> which only check with CONFIG_DEBUG_VM enabled, but this config may
>> impact performance, so it only for debug.
>>
>> Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
>> add the ability, which should be needed in any configs to catch the
>> invalid free, they even could be potential issue, eg, memory corruption,
>> use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
>> add dump_page() to help use to debug the issue.
> There are other situations in SLUB (such as with smaller allocations that
> don't go directly to page allocator) where use after free and double-free
> are undetected in non-debug configs, and it's expected that anyone debugging
> them will enable slub_debug or even DEBUG_VM. Why should this special case
> with nonslab pages be different?

I want the check back in kfree, this one is used  widely in driver, and 
the probability

of problem occurred is bigger in driver, especially in some out of tree 
drivers.

>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   mm/slub.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 3095b889fab4..555c2e6ccfca 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -3522,7 +3522,8 @@ static inline void free_nonslab_page(struct page *page, void *object)
>>   {
>>   	unsigned int order = compound_order(page);
>>   
>> -	VM_BUG_ON_PAGE(!PageCompound(page), page);
>> +	if (WARN_ON(!PageCompound(page)))
>> +		dump_page(page, "invalid free nonslab page");
>>   	kfree_hook(object);
>>   	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
>>   	__free_pages(page, order);
>>
> .
>

Re: [PATCH resend] slub: Add back check for free nonslab objects

[Matthew Wilcox]

On Mon, Sep 27, 2021 at 03:53:47PM +0800, Kefeng Wang wrote:
> On 2021/9/27 15:22, Vlastimil Babka wrote:
> > On 9/27/21 04:15, Kefeng Wang wrote:
> > > After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk
> > > free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE,
> > > which only check with CONFIG_DEBUG_VM enabled, but this config may
> > > impact performance, so it only for debug.
> > > 
> > > Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
> > > add the ability, which should be needed in any configs to catch the
> > > invalid free, they even could be potential issue, eg, memory corruption,
> > > use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
> > > add dump_page() to help use to debug the issue.
> > There are other situations in SLUB (such as with smaller allocations that
> > don't go directly to page allocator) where use after free and double-free
> > are undetected in non-debug configs, and it's expected that anyone debugging
> > them will enable slub_debug or even DEBUG_VM. Why should this special case
> > with nonslab pages be different?
> 
> I want the check back in kfree, this one is used  widely in driver, and the
> probability
> 
> of problem occurred is bigger in driver, especially in some out of tree
> drivers.

Why would we want to improve life for out of tree drivers?  Drivers should
be in-tree.  That's been the Linux Way for thirty years.

I remain sceptical that dump_page() is actually useful for debugging
drivers anyway.  dump_stack(), I could see -- that'll tell you which
driver called kfree() on a bogus pointer.  But how does dump_page() help?

Re: [PATCH resend] slub: Add back check for free nonslab objects

[Vlastimil Babka]

On 9/28/21 17:43, Matthew Wilcox wrote:
> On Mon, Sep 27, 2021 at 03:53:47PM +0800, Kefeng Wang wrote:
>> On 2021/9/27 15:22, Vlastimil Babka wrote:
>> > On 9/27/21 04:15, Kefeng Wang wrote:
>> > > After commit ("f227f0faf63b slub: fix unreclaimable slab stat for bulk
>> > > free"), the check for free nonslab page is replaced by VM_BUG_ON_PAGE,
>> > > which only check with CONFIG_DEBUG_VM enabled, but this config may
>> > > impact performance, so it only for debug.
>> > > 
>> > > Commit ("0937502af7c9 slub: Add check for kfree() of non slab objects.")
>> > > add the ability, which should be needed in any configs to catch the
>> > > invalid free, they even could be potential issue, eg, memory corruption,
>> > > use after free and double-free, so replace VM_BUG_ON_PAGE to WARN_ON, and
>> > > add dump_page() to help use to debug the issue.
>> > There are other situations in SLUB (such as with smaller allocations that
>> > don't go directly to page allocator) where use after free and double-free
>> > are undetected in non-debug configs, and it's expected that anyone debugging
>> > them will enable slub_debug or even DEBUG_VM. Why should this special case
>> > with nonslab pages be different?
>> 
>> I want the check back in kfree, this one is used  widely in driver, and the
>> probability
>> 
>> of problem occurred is bigger in driver, especially in some out of tree
>> drivers.
> 
> Why would we want to improve life for out of tree drivers?  Drivers should
> be in-tree.  That's been the Linux Way for thirty years.

Yes, there's a reason we distinguish VM_BUG_ON/VM_WARN_ON and plain
BUG_ON/WARN_ON. Picking arbitrarily one VM_ variant check and making it
always-enabled makes little sense to me, and doing it because of out of tree
drivers is certainly not a convincing argument. Commit f227f0faf63b was
correct in making it VM_BUG_ON.

> I remain sceptical that dump_page() is actually useful for debugging
> drivers anyway.  dump_stack(), I could see -- that'll tell you which
> driver called kfree() on a bogus pointer.  But how does dump_page() help?
>