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

[PATCH v2] 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, add
dump_page() and object address printing to help use to debug the issue.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v2: Add object address printing suggested by Matthew Wilcox

 mm/slub.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3095b889fab4..157973e22faf 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3522,7 +3522,11 @@ 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");
+		pr_warn("object pointer: 0x%p\n", object);
+	}
+
 	kfree_hook(object);
 	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
 	__free_pages(page, order);
-- 
2.26.2

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

[Shakeel Butt]

On Mon, Sep 27, 2021 at 5:24 AM Kefeng Wang <wangkefeng.wang@huawei.com> 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, add
> dump_page() and object address printing to help use to debug the issue.
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> v2: Add object address printing suggested by Matthew Wilcox
>
>  mm/slub.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 3095b889fab4..157973e22faf 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3522,7 +3522,11 @@ 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))) {

If there is a problem then this would be too noisy. Why not WARN_ON_ONCE()?

> +               dump_page(page, "invalid free nonslab page");
> +               pr_warn("object pointer: 0x%p\n", object);

Actually why not add 'once' semantics for the whole if-block?

> +       }
> +
>         kfree_hook(object);
>         mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
>         __free_pages(page, order);
> --
> 2.26.2
>

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

[Shakeel Butt]

On Mon, Sep 27, 2021 at 5:24 AM Kefeng Wang <wangkefeng.wang@huawei.com> 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, add
> dump_page() and object address printing to help use to debug the issue.
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> v2: Add object address printing suggested by Matthew Wilcox
>
>  mm/slub.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 3095b889fab4..157973e22faf 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3522,7 +3522,11 @@ 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))) {

If there is a problem then this would be too noisy. Why not WARN_ON_ONCE()?

> +               dump_page(page, "invalid free nonslab page");
> +               pr_warn("object pointer: 0x%p\n", object);

Actually why not add 'once' semantics for the whole if-block?

> +       }
> +
>         kfree_hook(object);
>         mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
>         __free_pages(page, order);
> --
> 2.26.2
>

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

[Kefeng Wang]

On 2021/9/28 23:09, Shakeel Butt wrote:
> On Mon, Sep 27, 2021 at 5:24 AM Kefeng Wang <wangkefeng.wang@huawei.com> 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, add
>> dump_page() and object address printing to help use to debug the issue.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>> v2: Add object address printing suggested by Matthew Wilcox
>>
>>   mm/slub.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 3095b889fab4..157973e22faf 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -3522,7 +3522,11 @@ 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))) {
> 
> If there is a problem then this would be too noisy. Why not WARN_ON_ONCE()?

If lots of abnormal/illegal pages are freed to freelist, the system
could be crash much more easier, with that in mind, I think the original 
logical use BUG_ON().

The ksize() use WARN_ON, looks no one report about too much log.

If we don't want too much dump, I will change it in v3.

> 
>> +               dump_page(page, "invalid free nonslab page");
>> +               pr_warn("object pointer: 0x%p\n", object);
> 
> Actually why not add 'once' semantics for the whole if-block?
> 
>> +       }
>> +
>>          kfree_hook(object);
>>          mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
>>          __free_pages(page, order);
>> --
>> 2.26.2
>>
> .
>