[PATCH 0/1] Fix slub_debug on 5.9-rc

[PATCH 0/1] Fix slub_debug on 5.9-rc

[Eric Farman]

Hi Vlastimil, et al,

Apologies in advance, for I am way out of my element at the moment.

I wanted to use slub_debug for a problem I'm looking at, and since
the slab in question is always the same I added this:

  slub_debug=FZ,dma-kmalloc-8

But then my system just crashes on bringup (using the 5.9 RCs):

[    6.712339] Unable to handle kernel pointer dereference in virtual kernel address space
[    6.712344] Failing address: 0000004400000000 TEID: 0000004400000803
[    6.712346] Fault in home space mode while using kernel ASCE.
[    6.712351] AS:00000000221ec007 R3:0000000000000024
[    6.712406] Oops: 003b ilc:3 [#1] SMP
[    6.712410] Modules linked in: pkey zcrypt rng_core
[    6.712415] CPU: 6 PID: 8 Comm: kworker/u564:0 Not tainted 5.9.0-rc7 #4
[    6.712418] Hardware name: IBM 2964 NE1 749 (LPAR)

The same option works fine on 5.8, so I bisected mm/ and ended up
blaming commit e17f1dfba37b ("mm, slub: extend slub_debug syntax for 
multiple blocks"). I didn't bother reverting this from 5.9, because
of the neighboring rework that this was a part of, but I did apply
it to 5.8, and recreated the problem there.

Looking at the above commit, I note that kmem_cache_flags() now
returns slub_debug instead of flags, which leads every kmem_cache
(other than one I'm trying to debug) getting its flags set to zero.
That wasn't the case previously, and seems quite suspect:

[    0.709206] s->name=dma-kmalloc-96 flags=4000 s->flags=0
[    0.709227] s->name=dma-kmalloc-192 flags=4000 s->flags=0
[    0.709248] s->name=dma-kmalloc-8 flags=4000 s->flags=4500
[    0.709269] s->name=dma-kmalloc-16 flags=4000 s->flags=0
[    0.709290] s->name=dma-kmalloc-32 flags=4000 s->flags=0

If I change my slab_list to "dma-kmalloc-*", and all dma slabs get
assigned the debug flags in addition to DMA, the panic seen on boot
disappears and my system comes up. I didn't leave my system up long
enoughu to see if weirdness on the other slabs occurred, but I
suspect it would.

I am running on s390, with defconfig, but I don't think that matters
here. Either way, the attached patch restores the variable that this
routine returns for slabs not affected by slub_debug, and lets my
system boot with various combinations of slub_debug parameters that
I've tried. Hopefully it's close to what is needed here.

I look forward to hearing your thoughts on this, and am happy to try
other things if I'm too far into the weeds. Thanks!

Eric Farman (1):
  mm, slub: Restore initial kmem_cache flags

 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/1] mm, slub: Restore initial kmem_cache flags

[Eric Farman]

The routine that applies debug flags to the kmem_cache slabs
inadvertantly prevents non-debug flags from being applied to
those same objects. That is, if slub_debug=<flag>,<slab> is
specified, non-debugged slabs will end up having flags of zero,
and the slabs will be unusable. Fix this by returning the input
flags for non-matching slabs as was done previously.

Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 mm/slub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index d4177aecedf6..3d7c95fd6a08 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1450,7 +1450,7 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 		}
 	}
 
-	return slub_debug;
+	return flags;
 }
 #else /* !CONFIG_SLUB_DEBUG */
 static inline void setup_object_debug(struct kmem_cache *s,
-- 
2.17.1

Re: [PATCH 1/1] mm, slub: Restore initial kmem_cache flags

[Vlastimil Babka]

On 9/30/20 1:26 PM, Eric Farman wrote:
> The routine that applies debug flags to the kmem_cache slabs
> inadvertantly prevents non-debug flags from being applied to
> those same objects. That is, if slub_debug=<flag>,<slab> is
> specified, non-debugged slabs will end up having flags of zero,
> and the slabs will be unusable. Fix this by returning the input
> flags for non-matching slabs as was done previously.

Thanks a lot for debugging this and sorry for the trouble!

> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

But with a small adjustment below:

> ---
>  mm/slub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index d4177aecedf6..3d7c95fd6a08 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1450,7 +1450,7 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>  		}
>  	}
>  
> -	return slub_debug;
> +	return flags;

To keep supporting the case of "debug flags set for all caches, with exceptions
for listed caches", i.e. "slub_debug=FZ;-,zs_handle,zspage", we should return
here this:

return flags | slub_debug;

Thanks again!

>  }
>  #else /* !CONFIG_SLUB_DEBUG */
>  static inline void setup_object_debug(struct kmem_cache *s,
> 

Re: [PATCH 1/1] mm, slub: Restore initial kmem_cache flags

[Eric Farman]

On 9/30/20 7:37 AM, Vlastimil Babka wrote:
> On 9/30/20 1:26 PM, Eric Farman wrote:
>> The routine that applies debug flags to the kmem_cache slabs
>> inadvertantly prevents non-debug flags from being applied to
>> those same objects. That is, if slub_debug=<flag>,<slab> is
>> specified, non-debugged slabs will end up having flags of zero,
>> and the slabs will be unusable. Fix this by returning the input
>> flags for non-matching slabs as was done previously.
> 
> Thanks a lot for debugging this and sorry for the trouble!

You're welcome. Just glad I wasn't losing my mind!

> 
>> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 
> But with a small adjustment below:
> 
>> ---
>>  mm/slub.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index d4177aecedf6..3d7c95fd6a08 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1450,7 +1450,7 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>>  		}
>>  	}
>>  
>> -	return slub_debug;
>> +	return flags;
> 
> To keep supporting the case of "debug flags set for all caches, with exceptions
> for listed caches", i.e. "slub_debug=FZ;-,zs_handle,zspage", we should return
> here this:
> 
> return flags | slub_debug;

Ah, cool...  I wondered about that, but didn't go far enough down the
combinations.  Does it then make sense to strip out the "if
(!slub_debug_string)" check at the beginning of the function?  As in:

----8<----

diff --git a/mm/slub.c b/mm/slub.c
index 3d7c95fd6a08..6d3574013b2f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1413,10 +1413,6 @@ slab_flags_t kmem_cache_flags(unsigned int
object_size,
        char *next_block;
        slab_flags_t block_flags;

-       /* If slub_debug = 0, it folds into the if conditional. */
-       if (!slub_debug_string)
-               return flags | slub_debug;
-
        len = strlen(name);
        next_block = slub_debug_string;
        /* Go through all blocks of debug options, see if any matches
our slab's name */
@@ -1450,7 +1446,7 @@ slab_flags_t kmem_cache_flags(unsigned int
object_size,
                }
        }

-       return flags;
+       return flags | slub_debug;
 }
 #else /* !CONFIG_SLUB_DEBUG */
 static inline void setup_object_debug(struct kmem_cache *s,

> 
> Thanks again!
> 
>>  }
>>  #else /* !CONFIG_SLUB_DEBUG */
>>  static inline void setup_object_debug(struct kmem_cache *s,
>>
> 

Re: [PATCH 1/1] mm, slub: Restore initial kmem_cache flags

[Vlastimil Babka]

On 9/30/20 3:06 PM, Eric Farman wrote:
> 
> 
> On 9/30/20 7:37 AM, Vlastimil Babka wrote:
>> On 9/30/20 1:26 PM, Eric Farman wrote:
>>> The routine that applies debug flags to the kmem_cache slabs
>>> inadvertantly prevents non-debug flags from being applied to
>>> those same objects. That is, if slub_debug=<flag>,<slab> is
>>> specified, non-debugged slabs will end up having flags of zero,
>>> and the slabs will be unusable. Fix this by returning the input
>>> flags for non-matching slabs as was done previously.
>> 
>> Thanks a lot for debugging this and sorry for the trouble!
> 
> You're welcome. Just glad I wasn't losing my mind!
> 
>> 
>>> Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> 
>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>> 
>> But with a small adjustment below:
>> 
>>> ---
>>>  mm/slub.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index d4177aecedf6..3d7c95fd6a08 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -1450,7 +1450,7 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>>>  		}
>>>  	}
>>>  
>>> -	return slub_debug;
>>> +	return flags;
>> 
>> To keep supporting the case of "debug flags set for all caches, with exceptions
>> for listed caches", i.e. "slub_debug=FZ;-,zs_handle,zspage", we should return
>> here this:
>> 
>> return flags | slub_debug;
> 
> Ah, cool...  I wondered about that, but didn't go far enough down the
> combinations.  Does it then make sense to strip out the "if
> (!slub_debug_string)" check at the beginning of the function?  As in:

Yeah, that makes sense.
Thanks!

[PATCH v2] mm, slub: Restore initial kmem_cache flags

[Eric Farman]

The routine that applies debug flags to the kmem_cache slabs
inadvertantly prevents non-debug flags from being applied to
those same objects. That is, if slub_debug=<flag>,<slab> is
specified, non-debugged slabs will end up having flags of zero,
and the slabs may be unusable.

Fix this by including the input flags for non-matching slabs
with the contents of slub_debug, so that the caches are
created as expected alongside any debugging options that may
be requested. With this, we can remove the check for a NULL
slub_debug_string, since it's covered by the loop itself.

Fixes: e17f1dfba37b ("mm, slub: extend slub_debug syntax for multiple blocks")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
---
 v2: OR return values per Vlastimil, applied his Ack
 v1: https://lore.kernel.org/linux-mm/20200930112612.76109-1-farman@linux.ibm.com/T/#t
---
 mm/slub.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index d4177aecedf6..6d3574013b2f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1413,10 +1413,6 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 	char *next_block;
 	slab_flags_t block_flags;
 
-	/* If slub_debug = 0, it folds into the if conditional. */
-	if (!slub_debug_string)
-		return flags | slub_debug;
-
 	len = strlen(name);
 	next_block = slub_debug_string;
 	/* Go through all blocks of debug options, see if any matches our slab's name */
@@ -1450,7 +1446,7 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 		}
 	}
 
-	return slub_debug;
+	return flags | slub_debug;
 }
 #else /* !CONFIG_SLUB_DEBUG */
 static inline void setup_object_debug(struct kmem_cache *s,
-- 
2.17.1