lkml.org 
[lkml]   [2022]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH 1/3] mm/slub: fix the race between validate_slab and slab_free
    From


    On 5/29/22 7:37 PM, Hyeonggon Yoo wrote:
    > On Sun, May 29, 2022 at 04:15:33PM +0800, Rongwei Wang wrote:
    >> In use cases where allocating and freeing slab frequently, some
    >> error messages, such as "Left Redzone overwritten", "First byte
    >> 0xbb instead of 0xcc" would be printed when validating slabs.
    >> That's because an object has been filled with SLAB_RED_INACTIVE,
    >> but has not been added to slab's freelist. And between these
    >> two states, the behaviour of validating slab is likely to occur.
    >>
    >> Actually, it doesn't mean the slab can not work stably. But, these
    >> confusing messages will disturb slab debugging more or less.
    >>
    >> Signed-off-by: Rongwei Wang <rongwei.wang@linux.alibaba.com>
    >
    > Have you observed it or it's from code inspection?
    Hi, Hyeonggon

    I try to build a module to trigger the race:

    #define SLUB_KTHREAD_MAX 1
    static int do_slub_alloc(void *data)
    {
    char *mm = NULL;
    char *mm1 = NULL;
    char *mm2 = NULL;
    char *mm3 = NULL;

    allow_signal(SIGTERM);

    while (1) {
    mm = kmalloc(2048, GFP_KERNEL);
    if (mm)
    mm[0x100] = 0x21;

    if (mm)
    kfree(mm);

    mm = NULL;
    if (kthread_should_stop())
    break;
    }

    return 0;
    }

    static int __init mini_init(void)
    {
    char *mm;
    int i = 0;
    unsigned int index;
    char kth_name[11] = "do_slub_00";

    for (i = 0; i < SLUB_KTHREAD_MAX; i++) {
    kth_name[9] = '0' + i%10;
    kth_name[8] = '0' + i/10;
    slub_thread[i] = kthread_run(do_slub_alloc, NULL,
    kth_name);
    }

    return 0;
    }
    module_init(mini_init);

    And in my system, I add 'slub_debug=UFPZ' to the boot options. Next, the
    error messages will be printed when I test "slabinfo -v" or "echo 1 >
    /sys/kernel/slab/kmalloc-2048/validate".

    >
    >> ---
    >> mm/slub.c | 40 +++++++++++++++++-----------------------
    >> 1 file changed, 17 insertions(+), 23 deletions(-)
    >>
    >> diff --git a/mm/slub.c b/mm/slub.c
    >> index ed5c2c03a47a..310e56d99116 100644
    >> --- a/mm/slub.c
    >> +++ b/mm/slub.c
    >> @@ -1374,15 +1374,12 @@ static noinline int free_debug_processing(
    >> void *head, void *tail, int bulk_cnt,
    >> unsigned long addr)
    >> {
    >> - struct kmem_cache_node *n = get_node(s, slab_nid(slab));
    >> void *object = head;
    >> int cnt = 0;
    >> - unsigned long flags, flags2;
    >> + unsigned long flags;
    >> int ret = 0;
    >>
    >> - spin_lock_irqsave(&n->list_lock, flags);
    >> - slab_lock(slab, &flags2);
    >> -
    >> + slab_lock(slab, &flags);
    >> if (s->flags & SLAB_CONSISTENCY_CHECKS) {
    >> if (!check_slab(s, slab))
    >> goto out;
    >> @@ -1414,8 +1411,7 @@ static noinline int free_debug_processing(
    >> slab_err(s, slab, "Bulk freelist count(%d) invalid(%d)\n",
    >> bulk_cnt, cnt);
    >>
    >> - slab_unlock(slab, &flags2);
    >> - spin_unlock_irqrestore(&n->list_lock, flags);
    >> + slab_unlock(slab, &flags);
    >> if (!ret)
    >> slab_fix(s, "Object at 0x%p not freed", object);
    >> return ret;
    >> @@ -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,15 +3311,19 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
    >> if (kfence_free(head))
    >> return;
    >>
    >> + n = get_node(s, slab_nid(slab));
    >> + spin_lock_irqsave(&n->list_lock, flags);
    >> +
    >
    > Oh please don't do this.
    >
    > SLUB free slowpath can be hit a lot depending on workload.
    Thanks, your words remind me. Actually, I put the original in
    free_debug_processing() lock on the outside of it. Looks this change is
    small. Indeed, it will degrade performance more or less.

    And do you have other ideas?:)

    -wrw
    >
    > __slab_free() try its best not to take n->list_lock. currently takes n->list_lock
    > only when the slab need to be taken from list.
    >
    > Unconditionally taking n->list_lock will degrade performance.
    >
    >> if (kmem_cache_debug(s) &&
    >> - !free_debug_processing(s, slab, head, tail, cnt, addr))
    >> + !free_debug_processing(s, slab, head, tail, cnt, addr)) {
    >> +
    >> + spin_unlock_irqrestore(&n->list_lock, flags);
    >> return;
    >> + }
    >>
    >> do {
    >> - if (unlikely(n)) {
    >> - spin_unlock_irqrestore(&n->list_lock, flags);
    >> - n = NULL;
    >> - }
    >> + if (unlikely(to_take_off))
    >> + to_take_off = 0;
    >> prior = slab->freelist;
    >> counters = slab->counters;
    >> set_freepointer(s, tail, prior);
    >> @@ -3343,18 +3343,11 @@ 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
    >> - * drop the list_lock without any processing.
    >> - *
    >> - * Otherwise the list_lock will synchronize with
    >> - * other processors updating the list of slabs.
    >> + * drop this behavior without any processing.
    >> */
    >> - spin_lock_irqsave(&n->list_lock, flags);
    >> -
    >> + to_take_off = 1;
    >> }
    >> }
    >>
    >> @@ -3363,8 +3356,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)) {
    >>
    >> + spin_unlock_irqrestore(&n->list_lock, flags);
    >> if (likely(was_frozen)) {
    >> /*
    >> * The list lock was not taken therefore no list
    >>
    >> --
    >> 2.27.0
    >>

    \
     
     \ /
      Last update: 2022-05-31 10:51    [W:4.492 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site