diff mbox series

[RFC,04/10] mm/hugetlb: Make userfaultfd_huge_must_wait() RCU-safe

Message ID 20221030212929.335473-5-peterx@redhat.com
State New
Headers
Series mm/hugetlb: Make huge_pte_offset() thread-safe for pmd unshare |

Commit Message

Peter Xu Oct. 30, 2022, 9:29 p.m. UTC 
  RCU makes sure the pte_t* won't go away from under us.  Please refer to the
comment above huge_pte_offset() for more information.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 fs/userfaultfd.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

James Houghton Nov. 2, 2022, 6:06 p.m. UTC | #1 
On Sun, Oct 30, 2022 at 2:29 PM Peter Xu <peterx@redhat.com> wrote:
>
> RCU makes sure the pte_t* won't go away from under us.  Please refer to the
> comment above huge_pte_offset() for more information.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  fs/userfaultfd.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 07c81ab3fd4d..4e813e68e4f8 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -243,6 +243,9 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
>
>         mmap_assert_locked(mm);
>
> +       /* For huge_pte_offset() */
> +       rcu_read_lock();

userfaultfd_huge_must_wait is called after we set the task's state to
blocking. Is it always safe to call rcu_read_lock (and
rcu_read_unlock) in this case? (With my basic understanding of RCU,
this seems like it should be safe, but I'm not sure.)

- James


> +
>         ptep = huge_pte_offset(mm, address, vma_mmu_pagesize(vma));
>
>         if (!ptep)
> @@ -261,6 +264,7 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
>         if (!huge_pte_write(pte) && (reason & VM_UFFD_WP))
>                 ret = true;
>  out:
> +       rcu_read_unlock();
>         return ret;
>  }
>  #else
> --
> 2.37.3
>
Peter Xu Nov. 2, 2022, 9:17 p.m. UTC | #2 
On Wed, Nov 02, 2022 at 11:06:16AM -0700, James Houghton wrote:
> On Sun, Oct 30, 2022 at 2:29 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > RCU makes sure the pte_t* won't go away from under us.  Please refer to the
> > comment above huge_pte_offset() for more information.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  fs/userfaultfd.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 07c81ab3fd4d..4e813e68e4f8 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -243,6 +243,9 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
> >
> >         mmap_assert_locked(mm);
> >
> > +       /* For huge_pte_offset() */
> > +       rcu_read_lock();
> 
> userfaultfd_huge_must_wait is called after we set the task's state to
> blocking. Is it always safe to call rcu_read_lock (and
> rcu_read_unlock) in this case? (With my basic understanding of RCU,
> this seems like it should be safe, but I'm not sure.)

I'm not aware of an issue here, but please shoot if you have any further
concerns or clues, because I'm definitely not a rcu person so I can
overlook things.

What I remember is my smoke test should be with LOCKDEP, it didn't trigger
anything so far I think.
diff mbox series

Patch

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 07c81ab3fd4d..4e813e68e4f8 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -243,6 +243,9 @@  static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
 
 	mmap_assert_locked(mm);
 
+	/* For huge_pte_offset() */
+	rcu_read_lock();
+
 	ptep = huge_pte_offset(mm, address, vma_mmu_pagesize(vma));
 
 	if (!ptep)
@@ -261,6 +264,7 @@  static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx,
 	if (!huge_pte_write(pte) && (reason & VM_UFFD_WP))
 		ret = true;
 out:
+	rcu_read_unlock();
 	return ret;
 }
 #else