| Message ID | 20221021163703.3218176-2-jthoughton@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hugetlb: introduce HugeTLB high-granularity mapping | expand |
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 > >
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 >
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 --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) {
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(-)