Re: [PATCH v2 24/33] mm/slob: Convert SLOB to use struct slab

[Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>]

On Wed, Dec 01, 2021 at 07:15:01PM +0100, Vlastimil Babka wrote:
> From: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
> 
> Use struct slab throughout the slob allocator.
> 
> [ vbabka@xxxxxxx: don't introduce wrappers for PageSlobFree in mm/slab.h just
>   for the single callers being wrappers in mm/slob.c ]
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
>  mm/slob.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/slob.c b/mm/slob.c
> index d2d15e7f191c..d3512bcc3141 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c

...

>  		/* Enough room on this page? */
> @@ -358,8 +358,8 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node,
>  		b = slob_new_pages(gfp & ~__GFP_ZERO, 0, node);
>  		if (!b)
>  			return NULL;
> -		sp = virt_to_page(b);
> -		__SetPageSlab(sp);
> +		sp = virt_to_slab(b);
> +		__SetPageSlab(slab_page(sp));

Hello Vlastimil.

I've tested this patch on my machine and it causes NULL pointer dereference.
that's because virt_to_slab returns NULL if folio_test_slab is false.
and __SetPageSlab is called with sp = NULL.

diff below fixed bug.

diff --git a/mm/slob.c b/mm/slob.c
index d3512bcc3141..cf669f03440f 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -358,8 +358,8 @@ static void *slob_alloc(size_t size, gfp_t gfp, int a
lign, int node,
                b = slob_new_pages(gfp & ~__GFP_ZERO, 0, node);
                if (!b)
                        return NULL;
+               __SetPageSlab(virt_to_page(b));
                sp = virt_to_slab(b);
-               __SetPageSlab(slab_page(sp));

                spin_lock_irqsave(&slob_lock, flags);
                sp->units = SLOB_UNITS(PAGE_SIZE);
 
 Thanks,
 Hyeonggon.

>  
>  		spin_lock_irqsave(&slob_lock, flags);
>  		sp->units = SLOB_UNITS(PAGE_SIZE);
> @@ -381,7 +381,7 @@ static void *slob_alloc(size_t size, gfp_t gfp, int align, int node,
>   */
>  static void slob_free(void *block, int size)
>  {
> -	struct page *sp;
> +	struct slab *sp;
>  	slob_t *prev, *next, *b = (slob_t *)block;
>  	slobidx_t units;
>  	unsigned long flags;
> @@ -391,7 +391,7 @@ static void slob_free(void *block, int size)
>  		return;
>  	BUG_ON(!size);
>  
> -	sp = virt_to_page(block);
> +	sp = virt_to_slab(block);
>  	units = SLOB_UNITS(size);
>  
>  	spin_lock_irqsave(&slob_lock, flags);
> @@ -401,8 +401,8 @@ static void slob_free(void *block, int size)
>  		if (slob_page_free(sp))
>  			clear_slob_page_free(sp);
>  		spin_unlock_irqrestore(&slob_lock, flags);
> -		__ClearPageSlab(sp);
> -		page_mapcount_reset(sp);
> +		__ClearPageSlab(slab_page(sp));
> +		page_mapcount_reset(slab_page(sp));
>  		slob_free_pages(b, 0);
>  		return;
>  	}
> -- 
> 2.33.1
> 
>