Re: [PATCH 1/3] mm/slub: fix the race between validate_slab and slab_free

[Rongwei Wang]

On 6/7/22 8:14 PM, Christoph Lameter wrote:
> On Fri, 3 Jun 2022, Rongwei Wang wrote:
>
> > Recently, I am also find other ways to solve this. That case was provided by
> > Muchun is useful (Thanks Muchun!). Indeed, it seems that use n->list_lock here
> > is unwise. Actually, I'm not sure if you recognize the existence of such race?
> > If all agrees this race, then the next question may be: do we want to solve
> > this problem? or as David said, it would be better to deprecate validate
> > attribute directly. I have no idea about it, hope to rely on your experience.
> >
> > In fact, I mainly want to collect your views on whether or how to fix this bug
> > here. Thanks!
>
>
> Well validate_slab() is rarely used and should not cause the hot paths to
> incur performance penalties. Fix it in the validation logic somehow? Or
> document the issue and warn that validation may not be correct if there
If available, I think document the issue and warn this incorrect 
behavior is OK. But it still prints a large amount of confusing 
messages, and disturbs us?
> are current operations on the slab being validated.
And I am trying to fix it in following way. In a short, these changes 
only works under the slub debug mode, and not affects the normal mode 
(I'm not sure). It looks not elegant enough. And if all approve of this 
way, I can submit the next version.

Anyway, thanks for your time:).
-wrw

@@ -3304,7 +3300,7 @@ static void __slab_free(struct kmem_cache *s, 
struct slab *slab,

 {
        void *prior;
-       int was_frozen;
+       int was_frozen, to_take_off = 0;
        struct slab new;
        unsigned long counters;
        struct kmem_cache_node *n = NULL;
@@ -3315,14 +3311,23 @@ static void __slab_free(struct kmem_cache *s, 
struct slab *slab,
        if (kfence_free(head))
                return;

-       if (kmem_cache_debug(s) &&
-           !free_debug_processing(s, slab, head, tail, cnt, addr))
-               return;
+       n = get_node(s, slab_nid(slab));
+       if (kmem_cache_debug(s)) {
+               int ret;

-       do {
-               if (unlikely(n)) {
+               spin_lock_irqsave(&n->list_lock, flags);
+               ret = free_debug_processing(s, slab, head, tail, cnt, addr);
+               if (!ret) {
                        spin_unlock_irqrestore(&n->list_lock, flags);
-                       n = NULL;
+                       return;
+               }
+       }
+
+       do {
+               if (unlikely(to_take_off)) {
+                       if (!kmem_cache_debug(s))
+                               spin_unlock_irqrestore(&n->list_lock, 
flags);
+                       to_take_off = 0;
                }
                prior = slab->freelist;
                counters = slab->counters;
@@ -3343,8 +3348,6 @@ static void __slab_free(struct kmem_cache *s, 
struct slab *slab,
                                new.frozen = 1;

                        } else { /* Needs to be taken off a list */
-
-                               n = get_node(s, slab_nid(slab));
                                /*
                                 * Speculatively acquire the list_lock.
                                 * If the cmpxchg does not succeed then 
we may
@@ -3353,8 +3356,10 @@ static void __slab_free(struct kmem_cache *s, 
struct slab *slab,
                                 * Otherwise the list_lock will 
synchronize with
                                 * other processors updating the list 
of slabs.
                                 */
-                               spin_lock_irqsave(&n->list_lock, flags);
+                               if (!kmem_cache_debug(s))
+                                       spin_lock_irqsave(&n->list_lock, 
flags);

+                               to_take_off = 1;
                        }
                }

@@ -3363,8 +3368,9 @@ static void __slab_free(struct kmem_cache *s, 
struct slab *slab,
                head, new.counters,
                "__slab_free"));

-       if (likely(!n)) {
-
+       if (likely(!to_take_off)) {
+               if (kmem_cache_debug(s))
+                       spin_unlock_irqrestore(&n->list_lock, flags);
                if (likely(was_frozen)) {
                        /*
                         * The list lock was not taken therefore no list
>