[v4,1/3] mm: Call arch_swap_restore() from do_swap_page()

  Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved
the call to swap_free() before the call to set_pte_at(), which meant that
the MTE tags could end up being freed before set_pte_at() had a chance
to restore them. Fix it by adding a call to the arch_swap_restore() hook
before the call to swap_free().

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965
Cc: <stable@vger.kernel.org> # 6.1
Fixes: c145e0b47c77 ("mm: streamline COW logic in do_swap_page()")
Reported-by: Qun-wei Lin (林群崴) <Qun-wei.Lin@mediatek.com>
Closes: https://lore.kernel.org/all/5050805753ac469e8d727c797c2218a9d780d434.camel@mediatek.com/
Acked-by: David Hildenbrand <david@redhat.com>
Acked-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
v2:
- Call arch_swap_restore() directly instead of via arch_do_swap_page()

 mm/memory.c | 7 +++++++
 1 file changed, 7 insertions(+)
  

Hi Peter,

On Mon, May 22, 2023 at 05:43:08PM -0700, Peter Collingbourne wrote:
> Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved
> the call to swap_free() before the call to set_pte_at(), which meant that
> the MTE tags could end up being freed before set_pte_at() had a chance
> to restore them. Fix it by adding a call to the arch_swap_restore() hook
> before the call to swap_free().
> 
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965
> Cc: <stable@vger.kernel.org> # 6.1
> Fixes: c145e0b47c77 ("mm: streamline COW logic in do_swap_page()")
> Reported-by: Qun-wei Lin (林群崴) <Qun-wei.Lin@mediatek.com>
> Closes: https://lore.kernel.org/all/5050805753ac469e8d727c797c2218a9d780d434.camel@mediatek.com/
> Acked-by: David Hildenbrand <david@redhat.com>
> Acked-by: "Huang, Ying" <ying.huang@intel.com>
> Reviewed-by: Steven Price <steven.price@arm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> v2:
> - Call arch_swap_restore() directly instead of via arch_do_swap_page()
> 
>  mm/memory.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index f69fbc251198..fc25764016b3 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3932,6 +3932,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>  		}
>  	}
>  
> +	/*
> +	 * Some architectures may have to restore extra metadata to the page
> +	 * when reading from swap. This metadata may be indexed by swap entry
> +	 * so this must be called before swap_free().
> +	 */
> +	arch_swap_restore(entry, folio);
> +
>  	/*
>  	 * Remove the swap entry and conditionally try to free up the swapcache.
>  	 * We're already holding a reference on the page but haven't mapped it

It looks like the intention is for this patch to land in 6.4, whereas the
other two in the series could go in later, right? If so, I was expecting
Andrew to pick this one up but he's not actually on CC. I've added him now,
but you may want to send this as a separate fix so it's obvious what needs
picking up for this cycle.

Cheers,

Will
  

On Mon, Jun 5, 2023 at 7:06 AM Will Deacon <will@kernel.org> wrote:
>
> Hi Peter,
>
> On Mon, May 22, 2023 at 05:43:08PM -0700, Peter Collingbourne wrote:
> > Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved
> > the call to swap_free() before the call to set_pte_at(), which meant that
> > the MTE tags could end up being freed before set_pte_at() had a chance
> > to restore them. Fix it by adding a call to the arch_swap_restore() hook
> > before the call to swap_free().
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965
> > Cc: <stable@vger.kernel.org> # 6.1
> > Fixes: c145e0b47c77 ("mm: streamline COW logic in do_swap_page()")
> > Reported-by: Qun-wei Lin (林群崴) <Qun-wei.Lin@mediatek.com>
> > Closes: https://lore.kernel.org/all/5050805753ac469e8d727c797c2218a9d780d434.camel@mediatek.com/
> > Acked-by: David Hildenbrand <david@redhat.com>
> > Acked-by: "Huang, Ying" <ying.huang@intel.com>
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> > v2:
> > - Call arch_swap_restore() directly instead of via arch_do_swap_page()
> >
> >  mm/memory.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index f69fbc251198..fc25764016b3 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -3932,6 +3932,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> >               }
> >       }
> >
> > +     /*
> > +      * Some architectures may have to restore extra metadata to the page
> > +      * when reading from swap. This metadata may be indexed by swap entry
> > +      * so this must be called before swap_free().
> > +      */
> > +     arch_swap_restore(entry, folio);
> > +
> >       /*
> >        * Remove the swap entry and conditionally try to free up the swapcache.
> >        * We're already holding a reference on the page but haven't mapped it
>
> It looks like the intention is for this patch to land in 6.4, whereas the
> other two in the series could go in later, right? If so, I was expecting
> Andrew to pick this one up but he's not actually on CC. I've added him now,
> but you may want to send this as a separate fix so it's obvious what needs
> picking up for this cycle.

I was expecting that this whole series could be picked up in mm. There
was a previous attempt to apply v3 of this series to mm, but that
failed because a dependent patch (commit c4c597f1b367 ("arm64: mte: Do
not set PG_mte_tagged if tags were not initialized")) hadn't been
merged into Linus's master branch yet. The series should be good to go
in now that that patch has been merged.

Peter
  

On Mon, Jun 05, 2023 at 10:41:12AM -0700, Peter Collingbourne wrote:
> On Mon, Jun 5, 2023 at 7:06 AM Will Deacon <will@kernel.org> wrote:
> > On Mon, May 22, 2023 at 05:43:08PM -0700, Peter Collingbourne wrote:
> > > Commit c145e0b47c77 ("mm: streamline COW logic in do_swap_page()") moved
> > > the call to swap_free() before the call to set_pte_at(), which meant that
> > > the MTE tags could end up being freed before set_pte_at() had a chance
> > > to restore them. Fix it by adding a call to the arch_swap_restore() hook
> > > before the call to swap_free().
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > Link: https://linux-review.googlesource.com/id/I6470efa669e8bd2f841049b8c61020c510678965
> > > Cc: <stable@vger.kernel.org> # 6.1
> > > Fixes: c145e0b47c77 ("mm: streamline COW logic in do_swap_page()")
> > > Reported-by: Qun-wei Lin (林群崴) <Qun-wei.Lin@mediatek.com>
> > > Closes: https://lore.kernel.org/all/5050805753ac469e8d727c797c2218a9d780d434.camel@mediatek.com/
> > > Acked-by: David Hildenbrand <david@redhat.com>
> > > Acked-by: "Huang, Ying" <ying.huang@intel.com>
> > > Reviewed-by: Steven Price <steven.price@arm.com>
> > > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > > ---
> > > v2:
> > > - Call arch_swap_restore() directly instead of via arch_do_swap_page()
> > >
> > >  mm/memory.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index f69fbc251198..fc25764016b3 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -3932,6 +3932,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> > >               }
> > >       }
> > >
> > > +     /*
> > > +      * Some architectures may have to restore extra metadata to the page
> > > +      * when reading from swap. This metadata may be indexed by swap entry
> > > +      * so this must be called before swap_free().
> > > +      */
> > > +     arch_swap_restore(entry, folio);
> > > +
> > >       /*
> > >        * Remove the swap entry and conditionally try to free up the swapcache.
> > >        * We're already holding a reference on the page but haven't mapped it
> >
> > It looks like the intention is for this patch to land in 6.4, whereas the
> > other two in the series could go in later, right? If so, I was expecting
> > Andrew to pick this one up but he's not actually on CC. I've added him now,
> > but you may want to send this as a separate fix so it's obvious what needs
> > picking up for this cycle.
> 
> I was expecting that this whole series could be picked up in mm. There
> was a previous attempt to apply v3 of this series to mm, but that
> failed because a dependent patch (commit c4c597f1b367 ("arm64: mte: Do
> not set PG_mte_tagged if tags were not initialized")) hadn't been
> merged into Linus's master branch yet. The series should be good to go
> in now that that patch has been merged.

Did this series fall through the cracks? I can't see it in linux-next
(or maybe my grep'ing failed). The commit mentioned above is in 6.4-rc3
AFAICT. Unfortunately Andrew was not cc'ed on the initial post, Will
added him later, so he likely missed it. For reference, the series is
here:

https://lore.kernel.org/r/20230523004312.1807357-1-pcc@google.com/

Andrew, what's your preference for this series? I'd like at least the
first patch to go into 6.5 as a fix. The second patch seems to be fairly
low risk and I'm happy for the third arm64 patch/cleanup to go in
6.5-rc1 (but it depends on the second patch). If you prefer, I can pick
them up and send a pull request to Linus next week before -rc1.
Otherwise you (or I) can queue the first patch and leave the other two
for 6.6.

Thanks.
  

On Thu, 29 Jun 2023 10:57:14 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:

> Andrew, what's your preference for this series? I'd like at least the
> first patch to go into 6.5 as a fix. The second patch seems to be fairly
> low risk and I'm happy for the third arm64 patch/cleanup to go in
> 6.5-rc1 (but it depends on the second patch). If you prefer, I can pick
> them up and send a pull request to Linus next week before -rc1.
> Otherwise you (or I) can queue the first patch and leave the other two
> for 6.6.

Thanks.  I queued [1/3] for 6.5-rcX with a cc:stable.  And I queued
[2/3] and [3/3] for 6.6-rc1.

If you wish to grab any/all of these then please do so - Stephen
will tell us of the duplicate and I'll drop the mm-git copy.
  

On Sun, Jul 02, 2023 at 12:38:21PM -0700, Andrew Morton wrote:
> On Thu, 29 Jun 2023 10:57:14 +0100 Catalin Marinas <catalin.marinas@arm.com> wrote:
> > Andrew, what's your preference for this series? I'd like at least the
> > first patch to go into 6.5 as a fix. The second patch seems to be fairly
> > low risk and I'm happy for the third arm64 patch/cleanup to go in
> > 6.5-rc1 (but it depends on the second patch). If you prefer, I can pick
> > them up and send a pull request to Linus next week before -rc1.
> > Otherwise you (or I) can queue the first patch and leave the other two
> > for 6.6.
> 
> Thanks.  I queued [1/3] for 6.5-rcX with a cc:stable.  And I queued
> [2/3] and [3/3] for 6.6-rc1.
> 
> If you wish to grab any/all of these then please do so - Stephen
> will tell us of the duplicate and I'll drop the mm-git copy.

That's great, thanks. We'll let you know if there are any conflicts
during the 6.6 preparation.