diff mbox series

[RFC,v2,01/47] hugetlb: don't set PageUptodate for UFFDIO_CONTINUE

Message ID 20221021163703.3218176-2-jthoughton@google.com
State New
Headers
Series hugetlb: introduce HugeTLB high-granularity mapping |

Commit Message

James Houghton Oct. 21, 2022, 4:36 p.m. UTC 
  This is how it should have been to begin with. It would be very bad if
we actually set PageUptodate with a UFFDIO_CONTINUE, as UFFDIO_CONTINUE
doesn't actually set/update the contents of the page, so we would be
exposing a non-zeroed page to the user.

The reason this change is being made now is because UFFDIO_CONTINUEs on
subpages definitely shouldn't set this page flag on the head page.

Signed-off-by: James Houghton <jthoughton@google.com>
---
 mm/hugetlb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Peter Xu Nov. 16, 2022, 4:30 p.m. UTC | #1 
On Fri, Oct 21, 2022 at 04:36:17PM +0000, James Houghton wrote:
> This is how it should have been to begin with. It would be very bad if
> we actually set PageUptodate with a UFFDIO_CONTINUE, as UFFDIO_CONTINUE
> doesn't actually set/update the contents of the page, so we would be
> exposing a non-zeroed page to the user.
> 
> The reason this change is being made now is because UFFDIO_CONTINUEs on
> subpages definitely shouldn't set this page flag on the head page.
> 
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>  mm/hugetlb.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1a7dc7b2e16c..650761cdd2f6 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6097,7 +6097,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>  	 * preceding stores to the page contents become visible before
>  	 * the set_pte_at() write.
>  	 */
> -	__SetPageUptodate(page);
> +	if (!is_continue)
> +		__SetPageUptodate(page);
> +	else
> +		VM_WARN_ON_ONCE_PAGE(!PageUptodate(page), page);

Yeah the old code looks wrong, I'm just wondering whether we can 100%
guarantee this for hugetlb.  E.g. for shmem that won't hold when we
uffd-continue on a not used page (e.g. by an over-sized fallocate()).

Another safer approach is simply fail the ioctl if !uptodate, but if you're
certain then WARN_ON_ONCE sounds all good too.  At least I did have a quick
look on hugetlb fallocate() and pages will be uptodate immediately.

>  
>  	/* Add shared, newly allocated pages to the page cache. */
>  	if (vm_shared && !is_continue) {
> -- 
> 2.38.0.135.g90850a2211-goog
> 
>
James Houghton Nov. 21, 2022, 6:33 p.m. UTC | #2 
On Wed, Nov 16, 2022 at 8:30 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 04:36:17PM +0000, James Houghton wrote:
> > This is how it should have been to begin with. It would be very bad if
> > we actually set PageUptodate with a UFFDIO_CONTINUE, as UFFDIO_CONTINUE
> > doesn't actually set/update the contents of the page, so we would be
> > exposing a non-zeroed page to the user.
> >
> > The reason this change is being made now is because UFFDIO_CONTINUEs on
> > subpages definitely shouldn't set this page flag on the head page.
> >
> > Signed-off-by: James Houghton <jthoughton@google.com>
> > ---
> >  mm/hugetlb.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 1a7dc7b2e16c..650761cdd2f6 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6097,7 +6097,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >        * preceding stores to the page contents become visible before
> >        * the set_pte_at() write.
> >        */
> > -     __SetPageUptodate(page);
> > +     if (!is_continue)
> > +             __SetPageUptodate(page);
> > +     else
> > +             VM_WARN_ON_ONCE_PAGE(!PageUptodate(page), page);
>
> Yeah the old code looks wrong, I'm just wondering whether we can 100%
> guarantee this for hugetlb.  E.g. for shmem that won't hold when we
> uffd-continue on a not used page (e.g. by an over-sized fallocate()).
>
> Another safer approach is simply fail the ioctl if !uptodate, but if you're
> certain then WARN_ON_ONCE sounds all good too.  At least I did have a quick
> look on hugetlb fallocate() and pages will be uptodate immediately.

Failing the ioctl sounds better than only WARNing. I'll do that and
drop the WARN_ON_ONCE for v1. Thanks!

- James

>
> >
> >       /* Add shared, newly allocated pages to the page cache. */
> >       if (vm_shared && !is_continue) {
> > --
> > 2.38.0.135.g90850a2211-goog
> >
> >
>
> --
> Peter Xu
>
Mike Kravetz Dec. 8, 2022, 10:55 p.m. UTC | #3 
On 11/21/22 10:33, James Houghton wrote:
> On Wed, Nov 16, 2022 at 8:30 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 04:36:17PM +0000, James Houghton wrote:
> > > This is how it should have been to begin with. It would be very bad if
> > > we actually set PageUptodate with a UFFDIO_CONTINUE, as UFFDIO_CONTINUE
> > > doesn't actually set/update the contents of the page, so we would be
> > > exposing a non-zeroed page to the user.
> > >
> > > The reason this change is being made now is because UFFDIO_CONTINUEs on
> > > subpages definitely shouldn't set this page flag on the head page.
> > >
> > > Signed-off-by: James Houghton <jthoughton@google.com>
> > > ---
> > >  mm/hugetlb.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > > index 1a7dc7b2e16c..650761cdd2f6 100644
> > > --- a/mm/hugetlb.c
> > > +++ b/mm/hugetlb.c
> > > @@ -6097,7 +6097,10 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > >        * preceding stores to the page contents become visible before
> > >        * the set_pte_at() write.
> > >        */
> > > -     __SetPageUptodate(page);
> > > +     if (!is_continue)
> > > +             __SetPageUptodate(page);
> > > +     else
> > > +             VM_WARN_ON_ONCE_PAGE(!PageUptodate(page), page);
> >
> > Yeah the old code looks wrong, I'm just wondering whether we can 100%
> > guarantee this for hugetlb.  E.g. for shmem that won't hold when we
> > uffd-continue on a not used page (e.g. by an over-sized fallocate()).
> >
> > Another safer approach is simply fail the ioctl if !uptodate, but if you're
> > certain then WARN_ON_ONCE sounds all good too.  At least I did have a quick
> > look on hugetlb fallocate() and pages will be uptodate immediately.
> 
> Failing the ioctl sounds better than only WARNing. I'll do that and
> drop the WARN_ON_ONCE for v1. Thanks!
> 

Sorry for the VERY late reply ...

After checking all the code paths, I do not think it is possible for a
!PageUptodate to be in the cache (target of continue).

ACK to failing the ioctl if not set, although I don't think it is possible
in current code.
diff mbox series

Patch

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 1a7dc7b2e16c..650761cdd2f6 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6097,7 +6097,10 @@  int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 	 * preceding stores to the page contents become visible before
 	 * the set_pte_at() write.
 	 */
-	__SetPageUptodate(page);
+	if (!is_continue)
+		__SetPageUptodate(page);
+	else
+		VM_WARN_ON_ONCE_PAGE(!PageUptodate(page), page);
 
 	/* Add shared, newly allocated pages to the page cache. */
 	if (vm_shared && !is_continue) {