| Message ID | 20210603180546.9083-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/swap: Fix pte_same_as_swp() not removing uffd-wp bit when compare | expand |
On Thu, 3 Jun 2021, Peter Xu wrote: > I found it by pure code review, that pte_same_as_swp() of unuse_vma() didn't Yes, that is an odd corner, easily missed. > take uffd-wp bit into account when comparing ptes. pte_same_as_swp() returning > false negative could cause failure to swapoff swap ptes that was wr-protected > by userfaultfd. > > Cc: Hugh Dickins <hughd@google.com> > Cc: Andrea Arcangeli <aarcange@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Peter Xu <peterx@redhat.com> I expect you're right: swapoff used to hang forever (but interruptibly) when this went wrong on powerpc originally. I don't know the uffd_wp (nor the soft_dirty) end of it, but treating uffd_wp and soft_dirty together looks a very good approach, so I'll venture an Acked-by: Hugh Dickins <hughd@google.com> But I think it should have a uffd_wp Fixes tag and be Cc stable. > --- > include/linux/swapops.h | 15 +++++++++++---- > mm/swapfile.c | 2 +- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h > index af3d2661e41e..d356ab4047f7 100644 > --- a/include/linux/swapops.h > +++ b/include/linux/swapops.h > @@ -23,6 +23,16 @@ > #define SWP_TYPE_SHIFT (BITS_PER_XA_VALUE - MAX_SWAPFILES_SHIFT) > #define SWP_OFFSET_MASK ((1UL << SWP_TYPE_SHIFT) - 1) > > +/* Clear all flags but only keep swp_entry_t related information */ > +static inline pte_t pte_swp_clear_flags(pte_t pte) > +{ > + if (pte_swp_soft_dirty(pte)) > + pte = pte_swp_clear_soft_dirty(pte); > + if (pte_swp_uffd_wp(pte)) > + pte = pte_swp_clear_uffd_wp(pte); > + return pte; > +} > + > /* > * Store a type+offset into a swp_entry_t in an arch-independent format > */ > @@ -66,10 +76,7 @@ static inline swp_entry_t pte_to_swp_entry(pte_t pte) > { > swp_entry_t arch_entry; > > - if (pte_swp_soft_dirty(pte)) > - pte = pte_swp_clear_soft_dirty(pte); > - if (pte_swp_uffd_wp(pte)) > - pte = pte_swp_clear_uffd_wp(pte); > + pte = pte_swp_clear_flags(pte); > arch_entry = __pte_to_swp_entry(pte); > return swp_entry(__swp_type(arch_entry), __swp_offset(arch_entry)); > } > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 6212bc033602..1e07d1c776f2 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1887,7 +1887,7 @@ unsigned int count_swap_pages(int type, int free) > > static inline int pte_same_as_swp(pte_t pte, pte_t swp_pte) > { > - return pte_same(pte_swp_clear_soft_dirty(pte), swp_pte); > + return pte_same(pte_swp_clear_flags(pte), swp_pte); > } > > /* > -- > 2.31.1
On Thu, Jun 03, 2021 at 08:26:02PM -0700, Hugh Dickins wrote: > On Thu, 3 Jun 2021, Peter Xu wrote: > > > I found it by pure code review, that pte_same_as_swp() of unuse_vma() didn't > > Yes, that is an odd corner, easily missed. > > > take uffd-wp bit into account when comparing ptes. pte_same_as_swp() returning > > false negative could cause failure to swapoff swap ptes that was wr-protected > > by userfaultfd. > > > > Cc: Hugh Dickins <hughd@google.com> > > Cc: Andrea Arcangeli <aarcange@redhat.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > I expect you're right: swapoff used to hang forever (but interruptibly) > when this went wrong on powerpc originally. I don't know the uffd_wp > (nor the soft_dirty) end of it, but treating uffd_wp and soft_dirty > together looks a very good approach, so I'll venture an > > Acked-by: Hugh Dickins <hughd@google.com> Thanks! > > But I think it should have a uffd_wp Fixes tag and be Cc stable. Yes, should be: Cc: stable@vger.kernel.org # v5.7+ Fixes: f45ec5ff16a7 ("userfaultfd: wp: support swap and page migration")
diff --git a/include/linux/swapops.h b/include/linux/swapops.h index af3d2661e41e..d356ab4047f7 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -23,6 +23,16 @@ #define SWP_TYPE_SHIFT (BITS_PER_XA_VALUE - MAX_SWAPFILES_SHIFT) #define SWP_OFFSET_MASK ((1UL << SWP_TYPE_SHIFT) - 1) +/* Clear all flags but only keep swp_entry_t related information */ +static inline pte_t pte_swp_clear_flags(pte_t pte) +{ + if (pte_swp_soft_dirty(pte)) + pte = pte_swp_clear_soft_dirty(pte); + if (pte_swp_uffd_wp(pte)) + pte = pte_swp_clear_uffd_wp(pte); + return pte; +} + /* * Store a type+offset into a swp_entry_t in an arch-independent format */ @@ -66,10 +76,7 @@ static inline swp_entry_t pte_to_swp_entry(pte_t pte) { swp_entry_t arch_entry; - if (pte_swp_soft_dirty(pte)) - pte = pte_swp_clear_soft_dirty(pte); - if (pte_swp_uffd_wp(pte)) - pte = pte_swp_clear_uffd_wp(pte); + pte = pte_swp_clear_flags(pte); arch_entry = __pte_to_swp_entry(pte); return swp_entry(__swp_type(arch_entry), __swp_offset(arch_entry)); } diff --git a/mm/swapfile.c b/mm/swapfile.c index 6212bc033602..1e07d1c776f2 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -1887,7 +1887,7 @@ unsigned int count_swap_pages(int type, int free) static inline int pte_same_as_swp(pte_t pte, pte_t swp_pte) { - return pte_same(pte_swp_clear_soft_dirty(pte), swp_pte); + return pte_same(pte_swp_clear_flags(pte), swp_pte); } /*
I found it by pure code review, that pte_same_as_swp() of unuse_vma() didn't take uffd-wp bit into account when comparing ptes. pte_same_as_swp() returning false negative could cause failure to swapoff swap ptes that was wr-protected by userfaultfd. Cc: Hugh Dickins <hughd@google.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Peter Xu <peterx@redhat.com> --- include/linux/swapops.h | 15 +++++++++++---- mm/swapfile.c | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-)