Re: [PATCH] mm/swap, PM: hibernate: atomically replace hibernation pin

[David CARLIER]

Hi CharSyam it is literally the same code, I did not look in existing
propositions when running the AI agent ... Sorry !

Kind regards.

On Sun, 3 May 2026 at 18:24, CharSyam <charsyam@xxxxxxxxx> wrote:
>
> Hi David, Youngjun,
>
> Thanks for the discussion.
>
> This is essentially the same direction as my v1, where I introduced a
> repin-style helper to transfer the SWP_HIBERNATION pin under a single
> swap_lock critical section.
>
> After Youngjun's review, I reworked it in v2 to avoid adding a new
> single-call-site helper and instead reused the existing
> find/pin/unpin helpers. The current v2 approach keeps the existing
> pin on failure and avoids introducing a new API.
>
> Given that this is more of a hardening improvement than a strict bug
> fix, I think the v2 approach is a simpler fit without adding extra API
> surface.
>
> Happy to adjust if there are specific concerns.
>
> Thanks,
> DaeMyung
>
>
> 2026년 5월 2일 (토) 오전 7:00, YoungJun Park <youngjun.park@xxxxxxx>님이 작성:
> >
> > On Thu, Apr 30, 2026 at 08:56:51PM +0100, David Carlier wrote:
> > > snapshot_set_swap_area() unpins the previously selected swap device
> > > and pins the new one in two separate swap_lock critical sections.
> > > In the gap between them, swapoff() observes SWP_HIBERNATION cleared,
> > > bypasses the guard, and tears down the device, reopening the race
> > > the SWP_HIBERNATION pin was meant to close. The window is reachable
> > > on any SNAPSHOT_SET_SWAP_AREA call after the snapshot device is
> > > opened for hibernation, and on any retry after the resume path's
> > > first selection.
> > >
> > > Add repin_hibernation_swap_type(), which looks up the new device,
> > > clears the old SWP_HIBERNATION flag and sets the new one under a
> > > single swap_lock acquisition. The same-device case is short-
> > > circuited so userspace can re-select the same swap area without
> > > tripping WARN_ON_ONCE and -EBUSY. Switch snapshot_set_swap_area()
> > > to the new helper.
> > >
> > > A failed lookup now preserves the previous pin instead of dropping
> > > it, so a bad SNAPSHOT_SET_SWAP_AREA leaves the prior selection
> > > intact. The open and release paths keep using
> > > pin_hibernation_swap_type() and unpin_hibernation_swap_type().
> > >
> > > The race was identified during AI-assisted review of the
> > > SWP_HIBERNATION pinning series.
> > >
> > > Fixes: 8e6e0d845823 ("mm/swap, PM: hibernate: fix swapoff race in uswsusp by pinning swap device")
> > > Assisted-by: Codex (gpt-5-codex)
> > > Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> > > ---
> > >  include/linux/swap.h |  2 ++
> > >  kernel/power/user.c  | 17 ++++--------
> > >  mm/swapfile.c        | 61 ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 68 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > index 1930f81e6be4..213ecb627a39 100644
> > > --- a/include/linux/swap.h
> > > +++ b/include/linux/swap.h
> > > @@ -436,6 +436,8 @@ static inline long get_nr_swap_pages(void)
> > >  extern void si_swapinfo(struct sysinfo *);
> > >  extern int pin_hibernation_swap_type(dev_t device, sector_t offset);
> > >  extern void unpin_hibernation_swap_type(int type);
> > > +extern int repin_hibernation_swap_type(int old_type, dev_t device,
> > > +                                       sector_t offset);
> > >  extern int find_hibernation_swap_type(dev_t device, sector_t offset);
> > >  int find_first_swap(dev_t *device);
> > >  extern unsigned int count_swap_pages(int, int);
> > > diff --git a/kernel/power/user.c b/kernel/power/user.c
> > > index d0fcfba7ac23..6e4f40e49319 100644
> > > --- a/kernel/power/user.c
> > > +++ b/kernel/power/user.c
> > > @@ -218,6 +218,7 @@ static int snapshot_set_swap_area(struct snapshot_data *data,
> > >  {
> > >       sector_t offset;
> > >       dev_t swdev;
> > > +     int new_type;
> > >
> > >       if (swsusp_swap_in_use())
> > >               return -EPERM;
> > > @@ -238,19 +239,11 @@ static int snapshot_set_swap_area(struct snapshot_data *data,
> > >               offset = swap_area.offset;
> > >       }
> > >
> > > -     /*
> > > -      * Unpin the swap device if a swap area was already
> > > -      * set by SNAPSHOT_SET_SWAP_AREA.
> > > -      */
> > > -     unpin_hibernation_swap_type(data->swap);
> > > +     new_type = repin_hibernation_swap_type(data->swap, swdev, offset);
> > > +     if (new_type < 0)
> > > +             return new_type;
> > >
> > > -     /*
> > > -      * User space encodes device types as two-byte values,
> > > -      * so we need to recode them
> > > -      */
> > > -     data->swap = pin_hibernation_swap_type(swdev, offset);
> > > -     if (data->swap < 0)
> > > -             return swdev ? -ENODEV : -EINVAL;
> > > +     data->swap = new_type;
> > >       data->dev = swdev;
> > >       return 0;
> > >  }
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index c7e173b93e11..4840fd40f36f 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -2219,6 +2219,67 @@ int pin_hibernation_swap_type(dev_t device, sector_t offset)
> > >       return type;
> > >  }
> > >
> > > +/**
> > > + * repin_hibernation_swap_type - Atomically replace the hibernation pin
> > > + * @old_type: Swap type currently pinned (or < 0 if none).
> > > + * @device:   Block device of the new resume image.
> > > + * @offset:   Offset identifying the new swap area.
> > > + *
> > > + * Look up the swap device for @device/@offset and atomically transfer
> > > + * the SWP_HIBERNATION pin from @old_type (if valid) to the new device,
> > > + * all under a single swap_lock critical section. This closes the
> > > + * swapoff() window that exists when callers unpin and re-pin in two
> > > + * separate operations.
> > > + *
> > > + * If the new device cannot be located, the existing pin on @old_type
> > > + * is preserved and an error is returned. If @old_type already refers
> > > + * to the same swap_info_struct as the new lookup, no flag changes are
> > > + * made and @old_type is returned.
> > > + *
> > > + * Return:
> > > + * >= 0 on success (new swap type).
> > > + * -EINVAL if @device is invalid.
> > > + * -ENODEV if the swap device is not found.
> > > + * -EBUSY  if the new device is already pinned by another context.
> > > + */
> > > +int repin_hibernation_swap_type(int old_type, dev_t device, sector_t offset)
> > > +{
> > > +     struct swap_info_struct *old_si, *new_si;
> > > +     int new_type;
> > > +
> > > +     spin_lock(&swap_lock);
> > > +
> > > +     new_type = __find_hibernation_swap_type(device, offset);
> > > +     if (new_type < 0) {
> > > +             spin_unlock(&swap_lock);
> > > +             return new_type;
> > > +     }
> > > +
> > > +     new_si = swap_type_to_info(new_type);
> > > +     if (WARN_ON_ONCE(!new_si)) {
> > > +             spin_unlock(&swap_lock);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     old_si = swap_type_to_info(old_type);
> > > +     if (new_si == old_si) {
> > > +             spin_unlock(&swap_lock);
> > > +             return new_type;
> > > +     }
> > > +
> > > +     if (WARN_ON_ONCE(new_si->flags & SWP_HIBERNATION)) {
> > > +             spin_unlock(&swap_lock);
> > > +             return -EBUSY;
> > > +     }
> > > +
> > > +     if (old_si)
> > > +             old_si->flags &= ~SWP_HIBERNATION;
> > > +     new_si->flags |= SWP_HIBERNATION;
> > > +
> > > +     spin_unlock(&swap_lock);
> > > +     return new_type;
> > > +}
> > > +
> > >  /**
> > >   * unpin_hibernation_swap_type - Unpin the swap device for hibernation
> > >   * @type: Swap type previously returned by pin_hibernation_swap_type()
> > > --
> > > 2.53.0
> >
> > I also caught up on this thread late due to travel.
> > Sorry.
> >
> > Hello David Carlier,
> >
> > It looks like the same issue and approach was raised previously:
> > https://lore.kernel.org/all/20260414143200.1267932-1-charsyam@xxxxxxxxx/#t
> > (+CC DaeMyung)
> >
> > I had suggested improving the fix by reusing the existing APIs
> > rather than adding a new one. v2 was posted along those lines:
> > https://lore.kernel.org/all/20260414164937.1363887-1-charsyam@xxxxxxxxx/
> >
> > To restate the points from that earlier review.
> >
> > 1. I think it is acceptable to release the existing pin when the
> >    new set operation fails. there is no significant side effect
> >    (see the earlier review for details).
> >
> > 2. If we still want to fix it, reusing the existing APIs seems
> >    preferable to adding a new one.
> >
> > Chris, Andrew - please take the prior thread into account during review.
> >
> > Thanks,
> > Youngjun Park
> >