Re: [PATCH 2/3] mm/swapfile.c: use swap_info[] to find the swap device

[Baoquan He <bhe@redhat.com>]

On 10/02/25 at 08:59am, Chris Li wrote:
> On Tue, Sep 30, 2025 at 9:35 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > Now, swap_active_head is only used to find a present swap device when
> > trying to swapoff it. In fact, swap_info[] is a short array which is
> > 32 at maximum, and usually the unused one can be reused, so the
> > searching for target mostly only iterates the foremost several used
> > slots. And swapoff is a rarely used operation, efficiency is not so
> > important. Then it's unnecessary to get a plist to make it.
> >
> > Here go by iterating swap_info[] to find the swap device instead of
> > iterating swap_active_head.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  mm/swapfile.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 5d71c748a2fe..18b52cc20749 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -2641,6 +2641,7 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >         struct inode *inode;
> >         struct filename *pathname;
> >         int err, found = 0;
> > +       unsigned int type;
> >
> >         if (!capable(CAP_SYS_ADMIN))
> >                 return -EPERM;
> > @@ -2658,7 +2659,8 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> >
> >         mapping = victim->f_mapping;
> >         spin_lock(&swap_lock);
> > -       plist_for_each_entry(p, &swap_active_head, list) {
> > +       for (type = 0; type < nr_swapfiles; type++) {
> > +               p = swap_info[type];
> 
> After some careful thinking of the swapoff behavior, I now consider
> this patch buggy.
> 
> On behavior change is that, previously the swapoff is dependent on the
> swap_active_list and only swapon device is on that list.
> 
> Now you change to enumerate every device at swap off and depend on the
> p->flags & SWP_WRITEOK to filter out the device. Which will go through
> all 32 swap devices regardless.
> 
> >                 if (p->flags & SWP_WRITEOK) {
> >                         if (p->swap_file->f_mapping == mapping) {
> 
> Considering the following race:
> 
> CPU1:
> swapoff( "/dev/sda1")
> spin_lock(&swap_lock);
> 
> for (type = 0; type < nr_swapfiles; type++) {
>      if (p->flags & SWP_WRITEOK) {
>          // found the device.
> }
> ....
> spin_lock(&p->lock);
> del_from_avail_list(p, true);
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> plist_del(&p->list, &swap_active_head);
> atomic_long_sub(p->pages, &nr_swap_pages);
> total_swap_pages -= p->pages;
> spin_unlock(&p->lock);
> spin_unlock(&swap_lock);
> 
> // at this point the p->flags still have SWP_WRITEOK.
> // swap_lock and p->lock are both unlocked.

No, SWP_WRITEOK is cleared in del_from_avail_list(). At this point,
there's no more SWP_WRITEOK in p->flags when both swap_lock and p->lock
are lifted.

> 
> Now the second CPU swapoff race begins
> 
>                                                        CPU2:
>                                                        swapoff( "/dev/sda1")
> 
> spin_lock(&swap_lock); // can take this lock
> 
>                                                        for (type = 0;
> type < nr_swapfiles; type++) {
>                                                        if (p->flags &
> SWP_WRITEOK) {
>                                                               // also
> found the device. because p->flags are not clear to 0 yet.
>                                                        }
>                                                        ....
> 
> spin_lock(&p->lock); // can also take this lock
> 
> del_from_avail_list(p, true);
> 
> plist_del(&p->list, &swap_active_head); <=== blow up here
>                                                        // remove p not
> the in list.
> 
> atomic_long_sub(p->pages, &nr_swap_pages);
>                                                        // double subtracting ..
> 
> total_swap_pages -= p->pages;
>                                                        // double
> subtracting as well.
>                                                        spin_unlock(&p->lock);
>                                                        spin_unlock(&swap_lock);
> 
> Please confirm or deny if that race is possible.
> If it is, consider it a NACK to this patch.
> 
> Please consider that even trivial patches can have subtle behavior
> change and it has non zero cost to reviewers. It can introduce subtle
> bugs unintentionally as well. In this case, I feel that the
> gain(removing a list, total less than 1K) is too small,  not a very
> good return on investment.
> 
> Chris
>