Re: [PATCH] Revert "mm, swap: avoid redundant swap device pinning"

["Huang, Ying" <ying.huang@linux.alibaba.com>]

Kairui Song <ryncsn@gmail.com> writes:

> On Mon, Nov 10, 2025 at 6:50 PM Huang, Ying
> <ying.huang@linux.alibaba.com> wrote:
>>
>> Kairui Song <ryncsn@gmail.com> writes:
>>
>> > On Mon, Nov 10, 2025 at 9:56 AM Huang, Ying
>> > <ying.huang@linux.alibaba.com> wrote:
>> >>
>> >> Hi, Kairui,
>> >>
>> >> Kairui Song via B4 Relay <devnull+kasong.tencent.com@kernel.org> writes:
>> >>
>> >> > From: Kairui Song <kasong@tencent.com>
>> >> >
>> >> > This reverts commit 78524b05f1a3e16a5d00cc9c6259c41a9d6003ce.
>> >> >
>> >> > While reviewing recent leaf entry changes, I noticed that commit
>> >> > 78524b05f1a3 ("mm, swap: avoid redundant swap device pinning") isn't
>> >> > correct. It's true that most all callers of __read_swap_cache_async are
>> >> > already holding a swap entry reference, so the repeated swap device
>> >> > pinning isn't needed on the same swap device, but it is possible that
>> >> > VMA readahead (swap_vma_readahead()) may encounter swap entries from a
>> >> > different swap device when there are multiple swap devices, and call
>> >> > __read_swap_cache_async without holding a reference to that swap device.
>> >> >
>> >> > So it is possible to cause a UAF if swapoff of device A raced with
>> >> > swapin on device B, and VMA readahead tries to read swap entries from
>> >> > device A. It's not easy to trigger but in theory possible to cause real
>> >> > issues. And besides, that commit made swap more vulnerable to issues
>> >> > like corrupted page tables.
>> >> >
>> >> > Just revert it. __read_swap_cache_async isn't that sensitive to
>> >> > performance after all, as it's mostly used for SSD/HDD swap devices with
>> >> > readahead. SYNCHRONOUS_IO devices may fallback onto it for swap count >
>> >> > 1 entries, but very soon we will have a new helper and routine for
>> >> > such devices, so they will never touch this helper or have redundant
>> >> > swap device reference overhead.
>> >>
>> >> Is it better to add get_swap_device() in swap_vma_readahead()?  Whenever
>> >> we get a swap entry, the first thing we need to do is call
>> >> get_swap_device() to check the validity of the swap entry and prevent
>> >> the backing swap device from going under us.  This helps us to avoid
>> >> checking the validity of the swap entry in every swap function.  Does
>> >> this sound reasonable?
>> >
>> > Hi Ying, thanks for the suggestion!
>> >
>> > Yes, that's also a feasible approach.
>> >
>> > What I was thinking is that, currently except the readahead path, all
>> > swapin entry goes through the get_swap_device() helper, that helper
>> > also helps to mitigate swap entry corruption that may causes OOB or
>> > NULL deref. Although I think it's really not that helpful at all to
>> > mitigate page table corruption from the kernel side, but seems not a
>> > really bad idea to have.
>> >
>> > And the code is simpler this way, and seems more suitable for a stable
>> > & mainline fix. If we want  to add get_swap_device() in
>> > swap_vma_readahead(), we need to do that for every entry that doesn't
>> > match the target entry's swap device. The reference overhead is
>> > trivial compared to readhead and bio layer, and only non
>> > SYNCHRONOUS_IO devices use this helper (madvise is a special case, we
>> > may optimize that later). ZRAM may fallback to the readahead path but
>> > this fallback will be eliminated very soon in swap table p2.
>>
>> We have 2 choices in general.
>>
>> 1. Add get/put_swap_device() in every swap function.
>>
>> 2. Add get/put_swap_device() in every caller of the swap functions.
>>
>> Personally, I prefer 2.  It works better in situations like calling
>> multiple swap functions.  It can reduce duplicated references.  It helps
>> improve code reasoning and readability.
>
> Totally agree, that's exactly what the recently added kerneldoc is
> suggesting, caller of the swap function will need to use refcount or
> lock to protect the swap device.
>
> I'm not suggesting to add get/put in every function, just thinking
> that maybe reverting it can have some nice side effects.
>
> But anyway, this fix should also be good:
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 3f85a1c4cfd9..4cca4865627f 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -747,6 +747,7 @@ static struct folio
> *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
>
>         blk_start_plug(&plug);
>         for (addr = start; addr < end; ilx++, addr += PAGE_SIZE) {
> +               struct swap_info_struct *si = NULL;
>                 leaf_entry_t entry;
>
>                 if (!pte++) {
> @@ -761,8 +762,12 @@ static struct folio
> *swap_vma_readahead(swp_entry_t targ_entry, gfp_t gfp_mask,
>                         continue;
>                 pte_unmap(pte);
>                 pte = NULL;
> +               if (swp_type(entry) != swp_type(targ_entry))
> +                       si = get_swap_device(entry);
>                 folio = __read_swap_cache_async(entry, gfp_mask, mpol, ilx,
>                                                 &page_allocated, false);
> +               if (si)
> +                       put_swap_device(si);
>                 if (!folio)
>                         continue;
>                 if (page_allocated) {
>
> I'll post a patch if it looks ok.

LGTM with the NULL check as you said in another email.

---
Best Regards,
Huang, Ying