Re: [PATCH] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap_info()

[Miaohe Lin]

On 2021/5/13 17:54, Muchun Song wrote:
> On Thu, May 13, 2021 at 5:11 PM Miaohe Lin <linmiaohe@xxxxxxxxxx> wrote:
>>
>> On 2021/5/13 14:48, Huang Ying wrote:
>>> Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array
>>> accesses to avoid NULL derefs"), the typical code to reference the
>>> swap_info[] is as follows,
>>>
>>>   type = swp_type(swp_entry);
>>>   if (type >= nr_swapfiles)
>>>           /* handle invalid swp_entry */;
>>>   p = swap_info[type];
>>>   /* access fields of *p.  OOPS! p may be NULL! */
>>>
>>> Because the ordering isn't guaranteed, it's possible that "p" is read
>>> before checking "type".  And that may result in NULL pointer
>>> dereference.
>>>
>>> So in commit c10d38cc8d3e, the code becomes,
>>>
>>>   struct swap_info_struct *swap_type_to_swap_info(int type)
>>>   {
>>>         if (type >= READ_ONCE(nr_swapfiles))
>>>                 return NULL;
>>>         smp_rmb();
>>>         return READ_ONCE(swap_info[type]);
>>>   }
>>>
>>>   /* users */
>>>   type = swp_type(swp_entry);
>>>   p = swap_type_to_swap_info(type);
>>>   if (!p)
>>>         /* handle invalid swp_entry */;
>>>   /* access fields of *p */
>>>
>>> Because "p" is checked to be non-zero before dereference, smp_rmb()
>>> isn't needed anymore.
>>>
>>> We still need to guarantee swap_info[type] is read before dereference.
>>> That can be satisfied via the data dependency ordering of
>>> READ_ONCE(swap_info[type]).  The corresponding smp_wmb() is adjusted
>>> in alloc_swap_info() too.
>>>
>>> And, we don't need to read "nr_swapfiles" too.  Because if
>>> "type >= nr_swapfiles", swap_info[type] will be NULL.  We just need
>>> to make sure we will not access out of the boundary of the array.
>>> With that change, nr_swapfiles will only be accessed with swap_lock
>>> held, except in swapcache_free_entries().  Where the absolute
>>> correctness of the value isn't needed, as described in the comments.
>>>
>>> Signed-off-by: "Huang, Ying" <ying.huang@xxxxxxxxx>
>>> Cc: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>
>>> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>>> Cc: Andrea Parri <andrea.parri@xxxxxxxxxxxxxxxxxxxx>
>>> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>>> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>>> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>>> Cc: Omar Sandoval <osandov@xxxxxx>
>>> Cc: Paul McKenney <paulmck@xxxxxxxxxx>
>>> Cc: Tejun Heo <tj@xxxxxxxxxx>
>>> Cc: Will Deacon <will.deacon@xxxxxxx>
>>> Cc: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>>> ---
>>>  mm/swapfile.c | 18 +++++++++---------
>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>>> index 2aad85751991..4c1fb28bbe0e 100644
>>> --- a/mm/swapfile.c
>>> +++ b/mm/swapfile.c
>>> @@ -100,10 +100,14 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0);
>>>
>>>  static struct swap_info_struct *swap_type_to_swap_info(int type)
>>>  {
>>> -     if (type >= READ_ONCE(nr_swapfiles))
>>> +     if (type >= MAX_SWAPFILES)
>>>               return NULL;
>>>
>>> -     smp_rmb();      /* Pairs with smp_wmb in alloc_swap_info. */
>>> +     /*
>>> +      * The data dependency ordering from the READ_ONCE() pairs
>>> +      * with smp_wmb() in alloc_swap_info() to guarantee the
>>> +      * swap_info_struct fields are read after swap_info[type].
>>> +      */
>>>       return READ_ONCE(swap_info[type]);
>>>  }
>>>
>>> @@ -2884,14 +2888,10 @@ static struct swap_info_struct *alloc_swap_info(void)
>>>       }
>>>       if (type >= nr_swapfiles) {
>>>               p->type = type;
>>> -             WRITE_ONCE(swap_info[type], p);
>>> -             /*
>>> -              * Write swap_info[type] before nr_swapfiles, in case a
>>> -              * racing procfs swap_start() or swap_next() is reading them.
>>> -              * (We never shrink nr_swapfiles, we never free this entry.)
>>> -              */
>>> +             /* Paired with READ_ONCE() in swap_type_to_swap_info() */
>>>               smp_wmb();
>>
>> Many thank for your patch. The patch looks fine to me. There is one question:
>>
>> There is no smp_rmb() paired with above smp_wmb(). What is this smp_wmb() used for ?
>> Could you please have a explanation ?
> 
> The comment is very clear, it matches READ_ONCE() which implies a
> data dependence barrier on some archs.
> 
> Thanks.

Got it. I misunderstood it... Thanks!

> 
>>
>> Thanks again!
>>
>>> -             WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1);
>>> +             WRITE_ONCE(swap_info[type], p);
>>> +             nr_swapfiles++;
>>>       } else {
>>>               defer = p;
>>>               p = swap_info[type];
>>>
>>
> .
>