linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] slub: fix unreclaimable slab stat for bulk free
@ 2021-07-28 15:53 Shakeel Butt
  2021-07-28 16:45 ` Michal Hocko
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Shakeel Butt @ 2021-07-28 15:53 UTC (permalink / raw)
  To: Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka
  Cc: Michal Hocko, Roman Gushchin, Wang Hai, Muchun Song,
	Andrew Morton, linux-mm, linux-kernel, Shakeel Butt

SLUB uses page allocator for higher order allocations and update
unreclaimable slab stat for such allocations. At the moment, the bulk
free for SLUB does not share code with normal free code path for these
type of allocations and have missed the stat update. So, fix the stat
update by common code. The user visible impact of the bug is the
potential of inconsistent unreclaimable slab stat visible through
meminfo and vmstat.

Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
Signed-off-by: Shakeel Butt <shakeelb@google.com>
---
 mm/slub.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 6dad2b6fda6f..03770291aa6b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3238,6 +3238,16 @@ struct detached_freelist {
 	struct kmem_cache *s;
 };
 
+static inline void free_nonslab_page(struct page *page)
+{
+	unsigned int order = compound_order(page);
+
+	VM_BUG_ON_PAGE(!PageCompound(page), page);
+	kfree_hook(page_address(page));
+	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
+	__free_pages(page, order);
+}
+
 /*
  * This function progressively scans the array with free objects (with
  * a limited look ahead) and extract objects belonging to the same
@@ -3274,9 +3284,7 @@ int build_detached_freelist(struct kmem_cache *s, size_t size,
 	if (!s) {
 		/* Handle kalloc'ed objects */
 		if (unlikely(!PageSlab(page))) {
-			BUG_ON(!PageCompound(page));
-			kfree_hook(object);
-			__free_pages(page, compound_order(page));
+			free_nonslab_page(page);
 			p[size] = NULL; /* mark object processed */
 			return size;
 		}
@@ -4252,13 +4260,7 @@ void kfree(const void *x)
 
 	page = virt_to_head_page(x);
 	if (unlikely(!PageSlab(page))) {
-		unsigned int order = compound_order(page);
-
-		BUG_ON(!PageCompound(page));
-		kfree_hook(object);
-		mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B,
-				      -(PAGE_SIZE << order));
-		__free_pages(page, order);
+		free_nonslab_page(page);
 		return;
 	}
 	slab_free(page->slab_cache, page, object, NULL, 1, _RET_IP_);
-- 
2.32.0.432.gabb21c7263-goog



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-07-28 15:53 [PATCH] slub: fix unreclaimable slab stat for bulk free Shakeel Butt
@ 2021-07-28 16:45 ` Michal Hocko
  2021-07-28 23:30 ` Roman Gushchin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Michal Hocko @ 2021-07-28 16:45 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka,
	Roman Gushchin, Wang Hai, Muchun Song, Andrew Morton, linux-mm,
	linux-kernel

On Wed 28-07-21 08:53:54, Shakeel Butt wrote:
> SLUB uses page allocator for higher order allocations and update
> unreclaimable slab stat for such allocations. At the moment, the bulk
> free for SLUB does not share code with normal free code path for these
> type of allocations and have missed the stat update. So, fix the stat
> update by common code. The user visible impact of the bug is the
> potential of inconsistent unreclaimable slab stat visible through
> meminfo and vmstat.
> 
> Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
> Signed-off-by: Shakeel Butt <shakeelb@google.com>

LGTM
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/slub.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 6dad2b6fda6f..03770291aa6b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3238,6 +3238,16 @@ struct detached_freelist {
>  	struct kmem_cache *s;
>  };
>  
> +static inline void free_nonslab_page(struct page *page)
> +{
> +	unsigned int order = compound_order(page);
> +
> +	VM_BUG_ON_PAGE(!PageCompound(page), page);
> +	kfree_hook(page_address(page));
> +	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
> +	__free_pages(page, order);
> +}
> +
>  /*
>   * This function progressively scans the array with free objects (with
>   * a limited look ahead) and extract objects belonging to the same
> @@ -3274,9 +3284,7 @@ int build_detached_freelist(struct kmem_cache *s, size_t size,
>  	if (!s) {
>  		/* Handle kalloc'ed objects */
>  		if (unlikely(!PageSlab(page))) {
> -			BUG_ON(!PageCompound(page));
> -			kfree_hook(object);
> -			__free_pages(page, compound_order(page));
> +			free_nonslab_page(page);
>  			p[size] = NULL; /* mark object processed */
>  			return size;
>  		}
> @@ -4252,13 +4260,7 @@ void kfree(const void *x)
>  
>  	page = virt_to_head_page(x);
>  	if (unlikely(!PageSlab(page))) {
> -		unsigned int order = compound_order(page);
> -
> -		BUG_ON(!PageCompound(page));
> -		kfree_hook(object);
> -		mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B,
> -				      -(PAGE_SIZE << order));
> -		__free_pages(page, order);
> +		free_nonslab_page(page);
>  		return;
>  	}
>  	slab_free(page->slab_cache, page, object, NULL, 1, _RET_IP_);
> -- 
> 2.32.0.432.gabb21c7263-goog

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-07-28 15:53 [PATCH] slub: fix unreclaimable slab stat for bulk free Shakeel Butt
  2021-07-28 16:45 ` Michal Hocko
@ 2021-07-28 23:30 ` Roman Gushchin
  2021-07-29  5:40 ` Muchun Song
  2021-07-29  6:52 ` Kefeng Wang
  3 siblings, 0 replies; 9+ messages in thread
From: Roman Gushchin @ 2021-07-28 23:30 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka,
	Michal Hocko, Wang Hai, Muchun Song, Andrew Morton, linux-mm,
	linux-kernel

On Wed, Jul 28, 2021 at 08:53:54AM -0700, Shakeel Butt wrote:
> SLUB uses page allocator for higher order allocations and update
> unreclaimable slab stat for such allocations. At the moment, the bulk
> free for SLUB does not share code with normal free code path for these
> type of allocations and have missed the stat update. So, fix the stat
> update by common code. The user visible impact of the bug is the
> potential of inconsistent unreclaimable slab stat visible through
> meminfo and vmstat.
> 
> Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
> Signed-off-by: Shakeel Butt <shakeelb@google.com>

Acked-by: Roman Gushchin <guro@fb.com>

Thanks!


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-07-28 15:53 [PATCH] slub: fix unreclaimable slab stat for bulk free Shakeel Butt
  2021-07-28 16:45 ` Michal Hocko
  2021-07-28 23:30 ` Roman Gushchin
@ 2021-07-29  5:40 ` Muchun Song
  2021-07-29  6:52 ` Kefeng Wang
  3 siblings, 0 replies; 9+ messages in thread
From: Muchun Song @ 2021-07-29  5:40 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka,
	Michal Hocko, Roman Gushchin, Wang Hai, Andrew Morton,
	Linux Memory Management List, LKML

On Wed, Jul 28, 2021 at 11:54 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> SLUB uses page allocator for higher order allocations and update
> unreclaimable slab stat for such allocations. At the moment, the bulk
> free for SLUB does not share code with normal free code path for these
> type of allocations and have missed the stat update. So, fix the stat
> update by common code. The user visible impact of the bug is the
> potential of inconsistent unreclaimable slab stat visible through
> meminfo and vmstat.
>
> Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
> Signed-off-by: Shakeel Butt <shakeelb@google.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-07-28 15:53 [PATCH] slub: fix unreclaimable slab stat for bulk free Shakeel Butt
                   ` (2 preceding siblings ...)
  2021-07-29  5:40 ` Muchun Song
@ 2021-07-29  6:52 ` Kefeng Wang
  2021-07-29 14:03   ` Shakeel Butt
  3 siblings, 1 reply; 9+ messages in thread
From: Kefeng Wang @ 2021-07-29  6:52 UTC (permalink / raw)
  To: Shakeel Butt, Christoph Lameter, Pekka Enberg, David Rientjes,
	Vlastimil Babka
  Cc: Michal Hocko, Roman Gushchin, Wang Hai, Muchun Song,
	Andrew Morton, linux-mm, linux-kernel


On 2021/7/28 23:53, Shakeel Butt wrote:
> SLUB uses page allocator for higher order allocations and update
> unreclaimable slab stat for such allocations. At the moment, the bulk
> free for SLUB does not share code with normal free code path for these
> type of allocations and have missed the stat update. So, fix the stat
> update by common code. The user visible impact of the bug is the
> potential of inconsistent unreclaimable slab stat visible through
> meminfo and vmstat.
>
> Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
> Signed-off-by: Shakeel Butt <shakeelb@google.com>
> ---
>   mm/slub.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 6dad2b6fda6f..03770291aa6b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3238,6 +3238,16 @@ struct detached_freelist {
>   	struct kmem_cache *s;
>   };
>   
> +static inline void free_nonslab_page(struct page *page)
> +{
> +	unsigned int order = compound_order(page);
> +
> +	VM_BUG_ON_PAGE(!PageCompound(page), page);

Could we add WARN_ON here, or we got nothing when CONFIG_DEBUG_VM is 
disabled.


> +	kfree_hook(page_address(page));
> +	mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B, -(PAGE_SIZE << order));
> +	__free_pages(page, order);
> +}
> +
>   /*
>    * This function progressively scans the array with free objects (with
>    * a limited look ahead) and extract objects belonging to the same
> @@ -3274,9 +3284,7 @@ int build_detached_freelist(struct kmem_cache *s, size_t size,
>   	if (!s) {
>   		/* Handle kalloc'ed objects */
>   		if (unlikely(!PageSlab(page))) {
> -			BUG_ON(!PageCompound(page));
> -			kfree_hook(object);
> -			__free_pages(page, compound_order(page));
> +			free_nonslab_page(page);
>   			p[size] = NULL; /* mark object processed */
>   			return size;
>   		}
> @@ -4252,13 +4260,7 @@ void kfree(const void *x)
>   
>   	page = virt_to_head_page(x);
>   	if (unlikely(!PageSlab(page))) {
> -		unsigned int order = compound_order(page);
> -
> -		BUG_ON(!PageCompound(page));
> -		kfree_hook(object);
> -		mod_lruvec_page_state(page, NR_SLAB_UNRECLAIMABLE_B,
> -				      -(PAGE_SIZE << order));
> -		__free_pages(page, order);
> +		free_nonslab_page(page);
>   		return;
>   	}
>   	slab_free(page->slab_cache, page, object, NULL, 1, _RET_IP_);


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-07-29  6:52 ` Kefeng Wang
@ 2021-07-29 14:03   ` Shakeel Butt
  2021-08-03 14:24     ` Kefeng Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Shakeel Butt @ 2021-07-29 14:03 UTC (permalink / raw)
  To: Kefeng Wang
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka,
	Michal Hocko, Roman Gushchin, Wang Hai, Muchun Song,
	Andrew Morton, Linux MM, LKML

On Wed, Jul 28, 2021 at 11:52 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
> On 2021/7/28 23:53, Shakeel Butt wrote:
> > SLUB uses page allocator for higher order allocations and update
> > unreclaimable slab stat for such allocations. At the moment, the bulk
> > free for SLUB does not share code with normal free code path for these
> > type of allocations and have missed the stat update. So, fix the stat
> > update by common code. The user visible impact of the bug is the
> > potential of inconsistent unreclaimable slab stat visible through
> > meminfo and vmstat.
> >
> > Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> > ---
> >   mm/slub.c | 22 ++++++++++++----------
> >   1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 6dad2b6fda6f..03770291aa6b 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3238,6 +3238,16 @@ struct detached_freelist {
> >       struct kmem_cache *s;
> >   };
> >
> > +static inline void free_nonslab_page(struct page *page)
> > +{
> > +     unsigned int order = compound_order(page);
> > +
> > +     VM_BUG_ON_PAGE(!PageCompound(page), page);
>
> Could we add WARN_ON here, or we got nothing when CONFIG_DEBUG_VM is
> disabled.

I don't have a strong opinion on this. Please send a patch with
reasoning if you want WARN_ON_ONCE here.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-07-29 14:03   ` Shakeel Butt
@ 2021-08-03 14:24     ` Kefeng Wang
  2021-08-03 14:29       ` Vlastimil Babka
  0 siblings, 1 reply; 9+ messages in thread
From: Kefeng Wang @ 2021-08-03 14:24 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Vlastimil Babka,
	Michal Hocko, Roman Gushchin, Wang Hai, Muchun Song,
	Andrew Morton, Linux MM, LKML


On 2021/7/29 22:03, Shakeel Butt wrote:
> On Wed, Jul 28, 2021 at 11:52 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>> On 2021/7/28 23:53, Shakeel Butt wrote:
>>> SLUB uses page allocator for higher order allocations and update
>>> unreclaimable slab stat for such allocations. At the moment, the bulk
>>> free for SLUB does not share code with normal free code path for these
>>> type of allocations and have missed the stat update. So, fix the stat
>>> update by common code. The user visible impact of the bug is the
>>> potential of inconsistent unreclaimable slab stat visible through
>>> meminfo and vmstat.
>>>
>>> Fixes: 6a486c0ad4dc ("mm, sl[ou]b: improve memory accounting")
>>> Signed-off-by: Shakeel Butt <shakeelb@google.com>
>>> ---
>>>    mm/slub.c | 22 ++++++++++++----------
>>>    1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index 6dad2b6fda6f..03770291aa6b 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -3238,6 +3238,16 @@ struct detached_freelist {
>>>        struct kmem_cache *s;
>>>    };
>>>
>>> +static inline void free_nonslab_page(struct page *page)
>>> +{
>>> +     unsigned int order = compound_order(page);
>>> +
>>> +     VM_BUG_ON_PAGE(!PageCompound(page), page);
>> Could we add WARN_ON here, or we got nothing when CONFIG_DEBUG_VM is
>> disabled.
> I don't have a strong opinion on this. Please send a patch with
> reasoning if you want WARN_ON_ONCE here.

Ok, we met a BUG_ON(!PageCompound(page)) in kfree() twice in lts4.4, we 
are still debugging it.

It's different to analyses due to no vmcore, and can't be reproduced.

WARN_ON() here could help us to notice the issue.

Also is there any experience or known fix/way to debug this kinds of 
issue? memory corruption?

Any suggestion will be appreciated, thanks.



> .
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-08-03 14:24     ` Kefeng Wang
@ 2021-08-03 14:29       ` Vlastimil Babka
  2021-08-03 14:44         ` Kefeng Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Vlastimil Babka @ 2021-08-03 14:29 UTC (permalink / raw)
  To: Kefeng Wang, Shakeel Butt
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Michal Hocko,
	Roman Gushchin, Wang Hai, Muchun Song, Andrew Morton, Linux MM,
	LKML

On 8/3/21 4:24 PM, Kefeng Wang wrote:
> 
> On 2021/7/29 22:03, Shakeel Butt wrote:
>> On Wed, Jul 28, 2021 at 11:52 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>
>>> On 2021/7/28 23:53, Shakeel Butt wrote:
>> I don't have a strong opinion on this. Please send a patch with
>> reasoning if you want WARN_ON_ONCE here.
> 
> Ok, we met a BUG_ON(!PageCompound(page)) in kfree() twice in lts4.4, we are
> still debugging it.
> 
> It's different to analyses due to no vmcore, and can't be reproduced.
> 
> WARN_ON() here could help us to notice the issue.
> 
> Also is there any experience or known fix/way to debug this kinds of issue?
> memory corruption?

This would typically be a use-after-free/double-free - a problem of the slab
user, not slab itself.

> Any suggestion will be appreciated, thanks.

debug_pagealloc could help to catch a use-after-free earlier

> 
>> .
>>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] slub: fix unreclaimable slab stat for bulk free
  2021-08-03 14:29       ` Vlastimil Babka
@ 2021-08-03 14:44         ` Kefeng Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Kefeng Wang @ 2021-08-03 14:44 UTC (permalink / raw)
  To: Vlastimil Babka, Shakeel Butt
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Michal Hocko,
	Roman Gushchin, Wang Hai, Muchun Song, Andrew Morton, Linux MM,
	LKML


On 2021/8/3 22:29, Vlastimil Babka wrote:
> On 8/3/21 4:24 PM, Kefeng Wang wrote:
>> On 2021/7/29 22:03, Shakeel Butt wrote:
>>> On Wed, Jul 28, 2021 at 11:52 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>>> On 2021/7/28 23:53, Shakeel Butt wrote:
>>> I don't have a strong opinion on this. Please send a patch with
>>> reasoning if you want WARN_ON_ONCE here.
>> Ok, we met a BUG_ON(!PageCompound(page)) in kfree() twice in lts4.4, we are
>> still debugging it.
>>
>> It's different to analyses due to no vmcore, and can't be reproduced.
>>
>> WARN_ON() here could help us to notice the issue.
>>
>> Also is there any experience or known fix/way to debug this kinds of issue?
>> memory corruption?
> This would typically be a use-after-free/double-free - a problem of the slab
> user, not slab itself.

We enable KASAN to find whether or not there are some UAF/double free, 
no related

log for now.

>
>> Any suggestion will be appreciated, thanks.
> debug_pagealloc could help to catch a use-after-free earlier

OK, will try this, thanks.

>
>>> .
>>>
> .
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-03 14:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-28 15:53 [PATCH] slub: fix unreclaimable slab stat for bulk free Shakeel Butt
2021-07-28 16:45 ` Michal Hocko
2021-07-28 23:30 ` Roman Gushchin
2021-07-29  5:40 ` Muchun Song
2021-07-29  6:52 ` Kefeng Wang
2021-07-29 14:03   ` Shakeel Butt
2021-08-03 14:24     ` Kefeng Wang
2021-08-03 14:29       ` Vlastimil Babka
2021-08-03 14:44         ` Kefeng Wang

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox