Re: [syzbot] [mm?] KASAN: slab-use-after-free Read in move_pages_pte

[Qi Zheng <zhengqi.arch@bytedance.com>]

On 2024/12/9 16:09, Qi Zheng wrote:
> 
> 
> On 2024/12/9 15:56, Yu Zhao wrote:
>> On Mon, Dec 9, 2024 at 12:00 AM Qi Zheng <zhengqi.arch@bytedance.com> 
>> wrote:
> 
> [...]
> 
>>>>>
>>>>> If you want syzbot to run the reproducer, reply with:
>>>>> #syz test: git://repo/address.git branch-or-commit-hash
>>>>> If you attach or paste a git patch, syzbot will apply it before 
>>>>> testing.
>>>
>>> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git
>>> mm-unstable
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 83fd35c034d7a..28526a4205d1b 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -7023,7 +7023,7 @@ static struct kmem_cache *page_ptl_cachep;
>>>    void __init ptlock_cache_init(void)
>>>    {
>>>           page_ptl_cachep = kmem_cache_create("page->ptl",
>>> sizeof(spinlock_t), 0,
>>> -                       SLAB_PANIC, NULL);
>>> +                       SLAB_PANIC|SLAB_TYPESAFE_BY_RCU, NULL);
>>
>> Note that `SLAB_TYPESAFE_BY_RCU` works by freeing the entire slab (the
>> page containing the objects) with RCU, not individual objects.
>>
>> So I don't think this would work. A PTL object can be re-allocated to
>> someone else, and that new user can re-initialize it. So trying to
>> concurrently lock it under RCU read lock would also be use-after-free.
>>
> 
> Got it. Thanks for pointing this out! So we should put ptlock_free()
> into the RCU callback instead of enabling SLAB_TYPESAFE_BY_RCU for
> page_ptl_cachep.

Like the following:

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 95bfaf5b85d90..b532415ef5841 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2988,7 +2988,7 @@ void ptlock_free(struct ptdesc *ptdesc);

  static inline spinlock_t *ptlock_ptr(struct ptdesc *ptdesc)
  {
-       return ptdesc->ptl;
+       return &(ptdesc->ptl->ptl);
  }
  #else /* ALLOC_SPLIT_PTLOCKS */
  static inline void ptlock_cache_init(void)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index d0e720ccecd71..7b94ea4d0d26a 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -434,6 +434,13 @@ FOLIO_MATCH(flags, _flags_2a);
  FOLIO_MATCH(compound_head, _head_2a);
  #undef FOLIO_MATCH

+#if ALLOC_SPLIT_PTLOCKS
+struct pt_lock {
+       spinlock_t ptl;
+       struct rcu_head rcu;
+};
+#endif
+
  /**
   * struct ptdesc -    Memory descriptor for page tables.
   * @__page_flags:     Same as page flags. Powerpc only.
@@ -478,7 +485,7 @@ struct ptdesc {
         union {
                 unsigned long _pt_pad_2;
  #if ALLOC_SPLIT_PTLOCKS
-               spinlock_t *ptl;
+               struct pt_lock *ptl;
  #else
                 spinlock_t ptl;
  #endif
diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
index a82aa80c0ba46..774ef2a128104 100644
--- a/include/linux/mm_types_task.h
+++ b/include/linux/mm_types_task.h
@@ -17,7 +17,8 @@
  #include <asm/tlbbatch.h>
  #endif

-#define ALLOC_SPLIT_PTLOCKS    (SPINLOCK_SIZE > BITS_PER_LONG/8)
+/*#define ALLOC_SPLIT_PTLOCKS  (SPINLOCK_SIZE > BITS_PER_LONG/8)*/
+#define ALLOC_SPLIT_PTLOCKS 1

  /*
   * When updating this, please also update struct resident_page_types[] in
diff --git a/mm/memory.c b/mm/memory.c
index 83fd35c034d7a..802dae0602b32 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -7022,24 +7022,34 @@ static struct kmem_cache *page_ptl_cachep;

  void __init ptlock_cache_init(void)
  {
-       page_ptl_cachep = kmem_cache_create("page->ptl", 
sizeof(spinlock_t), 0,
+       page_ptl_cachep = kmem_cache_create("page->ptl", sizeof(struct 
pt_lock), 0,
                         SLAB_PANIC, NULL);
  }

  bool ptlock_alloc(struct ptdesc *ptdesc)
  {
-       spinlock_t *ptl;
+       struct pt_lock *pt_lock;

-       ptl = kmem_cache_alloc(page_ptl_cachep, GFP_KERNEL);
-       if (!ptl)
+       pt_lock = kmem_cache_alloc(page_ptl_cachep, GFP_KERNEL);
+       if (!pt_lock)
                 return false;
-       ptdesc->ptl = ptl;
+       ptdesc->ptl = pt_lock;
         return true;
  }

+static void ptlock_free_rcu(struct rcu_head *head)
+{
+       struct pt_lock *pt_lock;
+
+       pt_lock = container_of(head, struct pt_lock, rcu);
+       kmem_cache_free(page_ptl_cachep, pt_lock);
+}
+
  void ptlock_free(struct ptdesc *ptdesc)
  {
-       kmem_cache_free(page_ptl_cachep, ptdesc->ptl);
+       struct pt_lock *pt_lock = ptdesc->ptl;
+
+       call_rcu(&pt_lock->rcu, ptlock_free_rcu);
  }
  #endif

> 
>>>