Re: [PATCH] mm/slab: always use cache from obj

Re: [PATCH] mm/slab: always use cache from obj

[Vlastimil Babka]

On 2/14/23 11:19, Jiazi.Li wrote:
> If free obj to a wrong cache, in addition random, different offset
> and object_size will also cause problems:
> 1. The offset of a cache with a ctor is not zero, free an object from
> this cache to cache with offset zero, will write next freepointer to
> wrong location, resulting in confusion of freelist.

Kernels hardened against freelist corruption will enable
CONFIG_SLAB_FREELIST_HARDENED, so that's already covered, no?

> 2. If wrong cache want init on free, and cache->object_size is large
> than obj size, which may lead to overwrite issue.

In general, being defensive against usage errors is part of either hardening
or debugging, which is what the existing code takes into account.

> Compared with adding a lot of if-else, it may be better to use obj's
> cache directly.
> 
> Signed-off-by: Jiazi.Li <jiazi.li@transsion.com>
> ---
>  mm/slab.h | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 63fb4c00d529..ed39b2e4f27b 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -670,10 +670,6 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>  {
>  	struct kmem_cache *cachep;
>  
> -	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> -	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> -		return s;
> -
>  	cachep = virt_to_cache(x);
>  	if (WARN(cachep && cachep != s,
>  		  "%s: Wrong slab cache. %s but object is from %s\n",

Re: [PATCH] mm/slab: always use cache from obj

[lijiazi]

On Tue, Feb 14, 2023 at 11:33:21AM +0100, Vlastimil Babka wrote:
> On 2/14/23 11:19, Jiazi.Li wrote:
> > If free obj to a wrong cache, in addition random, different offset
> > and object_size will also cause problems:
> > 1. The offset of a cache with a ctor is not zero, free an object from
> > this cache to cache with offset zero, will write next freepointer to
> > wrong location, resulting in confusion of freelist.
> 
> Kernels hardened against freelist corruption will enable
> CONFIG_SLAB_FREELIST_HARDENED, so that's already covered, no?
> 
Yes, HARDENED already covered.
> > 2. If wrong cache want init on free, and cache->object_size is large
> > than obj size, which may lead to overwrite issue.
> 
> In general, being defensive against usage errors is part of either hardening
> or debugging, which is what the existing code takes into account.
> 
My consideration is for the wrong cache problem on version without
HARDENED or debugging, it is likely to cause kernel panic, and such
problem is difficult to analyze on non-debug version.
When reproducing this problem on debug version, it will not cause kernel
panic, but only print the WARN log, then use correct cache to free obj.
Because we want to reproduce kernel panic problem, so may ignore WARN
log and think that can not reproduce problem on debug version.

Thanks for your reply, I will enable CONFIG_SLAB_FREELIST_HARDENED on
non-debug version later.
> > Compared with adding a lot of if-else, it may be better to use obj's
> > cache directly.
> > 
> > Signed-off-by: Jiazi.Li <jiazi.li@transsion.com>
> > ---
> >  mm/slab.h | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 63fb4c00d529..ed39b2e4f27b 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -670,10 +670,6 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> >  {
> >  	struct kmem_cache *cachep;
> >  
> > -	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> > -	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> > -		return s;
> > -
> >  	cachep = virt_to_cache(x);
> >  	if (WARN(cachep && cachep != s,
> >  		  "%s: Wrong slab cache. %s but object is from %s\n",
> 

Re: [PATCH] mm/slab: always use cache from obj

[Vlastimil Babka]

On 2/15/23 06:49, lijiazi wrote:
> On Tue, Feb 14, 2023 at 11:33:21AM +0100, Vlastimil Babka wrote:
>> On 2/14/23 11:19, Jiazi.Li wrote:
>> > If free obj to a wrong cache, in addition random, different offset
>> > and object_size will also cause problems:
>> > 1. The offset of a cache with a ctor is not zero, free an object from
>> > this cache to cache with offset zero, will write next freepointer to
>> > wrong location, resulting in confusion of freelist.
>> 
>> Kernels hardened against freelist corruption will enable
>> CONFIG_SLAB_FREELIST_HARDENED, so that's already covered, no?
>> 
> Yes, HARDENED already covered.
>> > 2. If wrong cache want init on free, and cache->object_size is large
>> > than obj size, which may lead to overwrite issue.
>> 
>> In general, being defensive against usage errors is part of either hardening
>> or debugging, which is what the existing code takes into account.
>> 
> My consideration is for the wrong cache problem on version without
> HARDENED or debugging, it is likely to cause kernel panic, and such
> problem is difficult to analyze on non-debug version.
> When reproducing this problem on debug version, it will not cause kernel
> panic, but only print the WARN log, then use correct cache to free obj.
> Because we want to reproduce kernel panic problem, so may ignore WARN
> log and think that can not reproduce problem on debug version.

If you need the panic in order to e.g. capture a crash dump, you could
enable slab debugging and boot with panic_on_warn to make the WARN result in
panic.

> Thanks for your reply, I will enable CONFIG_SLAB_FREELIST_HARDENED on
> non-debug version later.
>> > Compared with adding a lot of if-else, it may be better to use obj's
>> > cache directly.
>> > 
>> > Signed-off-by: Jiazi.Li <jiazi.li@transsion.com>
>> > ---
>> >  mm/slab.h | 4 ----
>> >  1 file changed, 4 deletions(-)
>> > 
>> > diff --git a/mm/slab.h b/mm/slab.h
>> > index 63fb4c00d529..ed39b2e4f27b 100644
>> > --- a/mm/slab.h
>> > +++ b/mm/slab.h
>> > @@ -670,10 +670,6 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
>> >  {
>> >  	struct kmem_cache *cachep;
>> >  
>> > -	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
>> > -	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
>> > -		return s;
>> > -
>> >  	cachep = virt_to_cache(x);
>> >  	if (WARN(cachep && cachep != s,
>> >  		  "%s: Wrong slab cache. %s but object is from %s\n",
>> 

Re: [PATCH] mm/slab: always use cache from obj

[lijiazi]

On Wed, Feb 15, 2023 at 10:41:32AM +0100, Vlastimil Babka wrote:
> On 2/15/23 06:49, lijiazi wrote:
> > On Tue, Feb 14, 2023 at 11:33:21AM +0100, Vlastimil Babka wrote:
> >> On 2/14/23 11:19, Jiazi.Li wrote:
> >> > If free obj to a wrong cache, in addition random, different offset
> >> > and object_size will also cause problems:
> >> > 1. The offset of a cache with a ctor is not zero, free an object from
> >> > this cache to cache with offset zero, will write next freepointer to
> >> > wrong location, resulting in confusion of freelist.
> >> 
> >> Kernels hardened against freelist corruption will enable
> >> CONFIG_SLAB_FREELIST_HARDENED, so that's already covered, no?
> >> 
> > Yes, HARDENED already covered.
> >> > 2. If wrong cache want init on free, and cache->object_size is large
> >> > than obj size, which may lead to overwrite issue.
> >> 
> >> In general, being defensive against usage errors is part of either hardening
> >> or debugging, which is what the existing code takes into account.
> >> 
> > My consideration is for the wrong cache problem on version without
> > HARDENED or debugging, it is likely to cause kernel panic, and such
> > problem is difficult to analyze on non-debug version.
> > When reproducing this problem on debug version, it will not cause kernel
> > panic, but only print the WARN log, then use correct cache to free obj.
> > Because we want to reproduce kernel panic problem, so may ignore WARN
> > log and think that can not reproduce problem on debug version.
> 
> If you need the panic in order to e.g. capture a crash dump, you could
> enable slab debugging and boot with panic_on_warn to make the WARN result in
> panic.
>

Thank you for your suggestion.
I used to think that the debug version has less tolerance for errors
than non-debug version, so I didn't add panic_on_warn.

> > Thanks for your reply, I will enable CONFIG_SLAB_FREELIST_HARDENED on
> > non-debug version later.
> >> > Compared with adding a lot of if-else, it may be better to use obj's
> >> > cache directly.
> >> > 
> >> > Signed-off-by: Jiazi.Li <jiazi.li@transsion.com>
> >> > ---
> >> >  mm/slab.h | 4 ----
> >> >  1 file changed, 4 deletions(-)
> >> > 
> >> > diff --git a/mm/slab.h b/mm/slab.h
> >> > index 63fb4c00d529..ed39b2e4f27b 100644
> >> > --- a/mm/slab.h
> >> > +++ b/mm/slab.h
> >> > @@ -670,10 +670,6 @@ static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
> >> >  {
> >> >  	struct kmem_cache *cachep;
> >> >  
> >> > -	if (!IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
> >> > -	    !kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS))
> >> > -		return s;
> >> > -
> >> >  	cachep = virt_to_cache(x);
> >> >  	if (WARN(cachep && cachep != s,
> >> >  		  "%s: Wrong slab cache. %s but object is from %s\n",
> >> 
>