diff mbox series

[34/46] hugetlb: userfaultfd: when using MADV_SPLIT, round addresses to PAGE_SIZE

Message ID 20230105101844.1893104-35-jthoughton@google.com
State New
Headers
Series Based on latest mm-unstable (85b44c25cd1e). |

Commit Message

James Houghton Jan. 5, 2023, 10:18 a.m. UTC 
  MADV_SPLIT enables HugeTLB HGM which allows for UFFDIO_CONTINUE in
PAGE_SIZE chunks. If a huge-page-aligned address were to be provided,
userspace would be completely unable to take advantage of HGM. That
would then require userspace to know to provide
UFFD_FEATURE_EXACT_ADDRESS.

This patch would make it harder to make a mistake. Instead of requiring
userspace to provide UFFD_FEATURE_EXACT_ADDRESS, always provide a usable
address.

Signed-off-by: James Houghton <jthoughton@google.com>
---
 mm/hugetlb.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

Peter Xu Jan. 6, 2023, 3:13 p.m. UTC | #1 
On Thu, Jan 05, 2023 at 10:18:32AM +0000, James Houghton wrote:
> MADV_SPLIT enables HugeTLB HGM which allows for UFFDIO_CONTINUE in
> PAGE_SIZE chunks. If a huge-page-aligned address were to be provided,
> userspace would be completely unable to take advantage of HGM. That
> would then require userspace to know to provide
> UFFD_FEATURE_EXACT_ADDRESS.
> 
> This patch would make it harder to make a mistake. Instead of requiring
> userspace to provide UFFD_FEATURE_EXACT_ADDRESS, always provide a usable
> address.
> 
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>  mm/hugetlb.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 5af6db52f34e..5b6215e03fe1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5936,28 +5936,27 @@ static inline vm_fault_t hugetlb_handle_userfault(struct vm_area_struct *vma,
>  						  unsigned long addr,
>  						  unsigned long reason)
>  {
> +	u32 hash;
> +	struct vm_fault vmf;
> +
>  	/*
>  	 * Don't use the hpage-aligned address if the user has explicitly
>  	 * enabled HGM.
>  	 */
>  	if (hugetlb_hgm_advised(vma) && reason == VM_UFFD_MINOR)
> -		haddr = address & PAGE_MASK;
> -
> -	u32 hash;
> -	struct vm_fault vmf = {
> -		.vma = vma,
> -		.address = haddr,
> -		.real_address = addr,
> -		.flags = flags,
> +		haddr = addr & PAGE_MASK;
>  
> -		/*
> -		 * Hard to debug if it ends up being
> -		 * used by a callee that assumes
> -		 * something about the other
> -		 * uninitialized fields... same as in
> -		 * memory.c
> -		 */
> -	};
> +	vmf.vma = vma;
> +	vmf.address = haddr;
> +	vmf.real_address = addr;
> +	vmf.flags = flags;

Const fields here:

mm/hugetlb.c: In function ‘hugetlb_handle_userfault’:
mm/hugetlb.c:5961:17: error: assignment of member ‘vma’ in read-only object
 5961 |         vmf.vma = vma;
      |                 ^ 
mm/hugetlb.c:5962:21: error: assignment of member ‘address’ in read-only object
 5962 |         vmf.address = haddr;   
      |                     ^      
mm/hugetlb.c:5963:26: error: assignment of member ‘real_address’ in read-only object                                                                                                          
 5963 |         vmf.real_address = addr;

> +	/*
> +	 * Hard to debug if it ends up being
> +	 * used by a callee that assumes
> +	 * something about the other
> +	 * uninitialized fields... same as in
> +	 * memory.c
> +	 */

PS: I think we can drop this along the way.

>  
>  	/*
>  	 * vma_lock and hugetlb_fault_mutex must be dropped before handling
> -- 
> 2.39.0.314.g84b9a713c41-goog
>
James Houghton Jan. 10, 2023, 2:50 p.m. UTC | #2 
On Fri, Jan 6, 2023 at 10:13 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Jan 05, 2023 at 10:18:32AM +0000, James Houghton wrote:
> > MADV_SPLIT enables HugeTLB HGM which allows for UFFDIO_CONTINUE in
> > PAGE_SIZE chunks. If a huge-page-aligned address were to be provided,
> > userspace would be completely unable to take advantage of HGM. That
> > would then require userspace to know to provide
> > UFFD_FEATURE_EXACT_ADDRESS.
> >
> > This patch would make it harder to make a mistake. Instead of requiring
> > userspace to provide UFFD_FEATURE_EXACT_ADDRESS, always provide a usable
> > address.
> >
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >  mm/hugetlb.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 5af6db52f34e..5b6215e03fe1 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -5936,28 +5936,27 @@ static inline vm_fault_t hugetlb_handle_userfault(struct vm_area_struct *vma,
> >                                                 unsigned long addr,
> >                                                 unsigned long reason)
> >  {
> > +     u32 hash;
> > +     struct vm_fault vmf;
> > +
> >       /*
> >        * Don't use the hpage-aligned address if the user has explicitly
> >        * enabled HGM.
> >        */
> >       if (hugetlb_hgm_advised(vma) && reason == VM_UFFD_MINOR)
> > -             haddr = address & PAGE_MASK;
> > -
> > -     u32 hash;
> > -     struct vm_fault vmf = {
> > -             .vma = vma,
> > -             .address = haddr,
> > -             .real_address = addr,
> > -             .flags = flags,
> > +             haddr = addr & PAGE_MASK;
> >
> > -             /*
> > -              * Hard to debug if it ends up being
> > -              * used by a callee that assumes
> > -              * something about the other
> > -              * uninitialized fields... same as in
> > -              * memory.c
> > -              */
> > -     };
> > +     vmf.vma = vma;
> > +     vmf.address = haddr;
> > +     vmf.real_address = addr;
> > +     vmf.flags = flags;
>
> Const fields here:
>
> mm/hugetlb.c: In function ‘hugetlb_handle_userfault’:
> mm/hugetlb.c:5961:17: error: assignment of member ‘vma’ in read-only object
>  5961 |         vmf.vma = vma;
>       |                 ^
> mm/hugetlb.c:5962:21: error: assignment of member ‘address’ in read-only object
>  5962 |         vmf.address = haddr;
>       |                     ^
> mm/hugetlb.c:5963:26: error: assignment of member ‘real_address’ in read-only object
>  5963 |         vmf.real_address = addr;

Thanks Peter for this and your other findings. Not sure why my
compiler (clang) let me do this. :/ Will send a v2 soon with this +
the other problems fixed.
diff mbox series

Patch

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5af6db52f34e..5b6215e03fe1 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5936,28 +5936,27 @@  static inline vm_fault_t hugetlb_handle_userfault(struct vm_area_struct *vma,
 						  unsigned long addr,
 						  unsigned long reason)
 {
+	u32 hash;
+	struct vm_fault vmf;
+
 	/*
 	 * Don't use the hpage-aligned address if the user has explicitly
 	 * enabled HGM.
 	 */
 	if (hugetlb_hgm_advised(vma) && reason == VM_UFFD_MINOR)
-		haddr = address & PAGE_MASK;
-
-	u32 hash;
-	struct vm_fault vmf = {
-		.vma = vma,
-		.address = haddr,
-		.real_address = addr,
-		.flags = flags,
+		haddr = addr & PAGE_MASK;
 
-		/*
-		 * Hard to debug if it ends up being
-		 * used by a callee that assumes
-		 * something about the other
-		 * uninitialized fields... same as in
-		 * memory.c
-		 */
-	};
+	vmf.vma = vma;
+	vmf.address = haddr;
+	vmf.real_address = addr;
+	vmf.flags = flags;
+	/*
+	 * Hard to debug if it ends up being
+	 * used by a callee that assumes
+	 * something about the other
+	 * uninitialized fields... same as in
+	 * memory.c
+	 */
 
 	/*
 	 * vma_lock and hugetlb_fault_mutex must be dropped before handling