We touch slab->freelist and slab->inuse before checking the slab pointer
is actually sane. Do that validation first, which will be safer. We can
thus also remove the check from alloc_debug_processing().
This adds a new "s->flags & SLAB_CONSISTENCY_CHECKS" test but
alloc_single_from_partial() is only called for caches with debugging
enabled so it's acceptable.
In alloc_single_from_new_slab() we just created the struct slab and call
alloc_debug_processing() to mainly set up redzones, tracking etc, while
not really expecting the consistency checks to fail. Thus don't validate
it there.
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/slub.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index 12ad42f3d2e066b02340f2c30a85422583af3c5d..e5b53d1debddd3fe0f941f579a1043a5b976e50b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -821,6 +821,8 @@ static inline unsigned int get_orig_size(struct kmem_cache *s, void *object)
return *(unsigned int *)p;
}
+#ifdef CONFIG_SLUB_DEBUG
+
/*
* For debugging context when we want to check if the struct slab pointer
* appears to be valid.
@@ -830,7 +832,6 @@ static inline bool validate_slab_ptr(struct slab *slab)
return PageSlab(slab_page(slab));
}
-#ifdef CONFIG_SLUB_DEBUG
static unsigned long object_map[BITS_TO_LONGS(MAX_OBJS_PER_PAGE)];
static DEFINE_SPINLOCK(object_map_lock);
@@ -1651,11 +1652,6 @@ static noinline bool alloc_debug_processing(struct kmem_cache *s,
struct slab *slab, void *object, int orig_size)
{
if (s->flags & SLAB_CONSISTENCY_CHECKS) {
- if (!validate_slab_ptr(slab)) {
- slab_err(s, slab, "Not a valid slab page");
- return false;
- }
-
if (!alloc_consistency_checks(s, slab, object))
goto bad;
}
@@ -2825,13 +2821,21 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
lockdep_assert_held(&n->list_lock);
+#ifdef SLUB_DEBUG
+ if (s->flags & SLAB_CONSISTENCY_CHECKS) {
+ if (!validate_slab_ptr(slab)) {
+ slab_err(s, slab, "Not a valid slab page");
+ return NULL;
+ }
+ }
+#endif
+
object = slab->freelist;
slab->freelist = get_freepointer(s, object);
slab->inuse++;
if (!alloc_debug_processing(s, slab, object, orig_size)) {
- if (validate_slab_ptr(slab))
- remove_partial(n, slab);
+ remove_partial(n, slab);
return NULL;
}
--
2.51.0On Mon, Sep 15, 2025 at 03:55:12PM +0200, Vlastimil Babka wrote:
> We touch slab->freelist and slab->inuse before checking the slab pointer
> is actually sane. Do that validation first, which will be safer. We can
> thus also remove the check from alloc_debug_processing().
>
> This adds a new "s->flags & SLAB_CONSISTENCY_CHECKS" test but
> alloc_single_from_partial() is only called for caches with debugging
> enabled so it's acceptable.
>
> In alloc_single_from_new_slab() we just created the struct slab and call
> alloc_debug_processing() to mainly set up redzones, tracking etc, while
> not really expecting the consistency checks to fail. Thus don't validate
> it there.
>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> mm/slub.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 12ad42f3d2e066b02340f2c30a85422583af3c5d..e5b53d1debddd3fe0f941f579a1043a5b976e50b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -821,6 +821,8 @@ static inline unsigned int get_orig_size(struct kmem_cache *s, void *object)
> return *(unsigned int *)p;
> }
>
> +#ifdef CONFIG_SLUB_DEBUG
> +
> /*
> * For debugging context when we want to check if the struct slab pointer
> * appears to be valid.
> @@ -830,7 +832,6 @@ static inline bool validate_slab_ptr(struct slab *slab)
> return PageSlab(slab_page(slab));
> }
>
> -#ifdef CONFIG_SLUB_DEBUG
> static unsigned long object_map[BITS_TO_LONGS(MAX_OBJS_PER_PAGE)];
> static DEFINE_SPINLOCK(object_map_lock);
>
> @@ -1651,11 +1652,6 @@ static noinline bool alloc_debug_processing(struct kmem_cache *s,
> struct slab *slab, void *object, int orig_size)
> {
> if (s->flags & SLAB_CONSISTENCY_CHECKS) {
> - if (!validate_slab_ptr(slab)) {
> - slab_err(s, slab, "Not a valid slab page");
> - return false;
> - }
> -
> if (!alloc_consistency_checks(s, slab, object))
> goto bad;
> }
> @@ -2825,13 +2821,21 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
>
> lockdep_assert_held(&n->list_lock);
>
> +#ifdef SLUB_DEBUG
I'm sure you meant CONFIG_SLUB_DEBUG ;)
With that adjusted, looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
> + if (s->flags & SLAB_CONSISTENCY_CHECKS) {
> + if (!validate_slab_ptr(slab)) {
> + slab_err(s, slab, "Not a valid slab page");
> + return NULL;
> + }
> + }
> +#endif
> +
> object = slab->freelist;
> slab->freelist = get_freepointer(s, object);
> slab->inuse++;
>
> if (!alloc_debug_processing(s, slab, object, orig_size)) {
> - if (validate_slab_ptr(slab))
> - remove_partial(n, slab);
> + remove_partial(n, slab);
> return NULL;
> }
--
Cheers,
Harry / Hyeonggon
On 9/15/25 16:25, Harry Yoo wrote:
> On Mon, Sep 15, 2025 at 03:55:12PM +0200, Vlastimil Babka wrote:
>> @@ -2825,13 +2821,21 @@ static void *alloc_single_from_partial(struct kmem_cache *s,
>>
>> lockdep_assert_held(&n->list_lock);
>>
>> +#ifdef SLUB_DEBUG
>
> I'm sure you meant CONFIG_SLUB_DEBUG ;)
>
> With that adjusted, looks good to me,
> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
Doh yes, thanks a lot :)
>> + if (s->flags & SLAB_CONSISTENCY_CHECKS) {
>> + if (!validate_slab_ptr(slab)) {
>> + slab_err(s, slab, "Not a valid slab page");
>> + return NULL;
>> + }
>> + }
>> +#endif
>> +
>> object = slab->freelist;
>> slab->freelist = get_freepointer(s, object);
>> slab->inuse++;
>>
>> if (!alloc_debug_processing(s, slab, object, orig_size)) {
>> - if (validate_slab_ptr(slab))
>> - remove_partial(n, slab);
>> + remove_partial(n, slab);
>> return NULL;
>> }
>
© 2016 - 2025 Red Hat, Inc.