| Message ID | 20220914190917.3517663-1-opendmb@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/hugetlb: correct demote page offset logic | expand |
On Wed, 14 Sep 2022 12:09:17 -0700 Doug Berger <opendmb@gmail.com> wrote: > With gigantic pages it may not be true that struct page structures > are contiguous across the entire gigantic page. The nth_page macro > is used here in place of direct pointer arithmetic to correct for > this. What were the user-visible runtime effects of this bug?
On 09/14/22 12:09, Doug Berger wrote: > With gigantic pages it may not be true that struct page structures > are contiguous across the entire gigantic page. The nth_page macro > is used here in place of direct pointer arithmetic to correct for > this. > > Fixes: 8531fc6f52f5 ("hugetlb: add hugetlb demote page support") > Signed-off-by: Doug Berger <opendmb@gmail.com> > Cc: <stable@vger.kernel.org> > --- > mm/hugetlb.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) Thanks! Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> To answer Andrew's question about user-visible runtime effects. We could get addressing exceptions. However, this is only possible in configurations where CONFIG_SPARSEMEM && !CONFIG_SPARSEMEM_VMEMMAP. Such a configuration option is rare an unknown to be the default anywhere.
On 9/14/2022 1:49 PM, Andrew Morton wrote: > On Wed, 14 Sep 2022 12:09:17 -0700 Doug Berger <opendmb@gmail.com> wrote: > >> With gigantic pages it may not be true that struct page structures >> are contiguous across the entire gigantic page. The nth_page macro >> is used here in place of direct pointer arithmetic to correct for >> this. > > What were the user-visible runtime effects of this bug? As Mike said this would only conceptually be a problem for systems with CONFIG_SPARSEMEM && !CONFIG_SPARSEMEM_VMEMMAP, and could cause kernel address exceptions or memory corruption with unpredictable side effects. However, I am unaware of a system other than perhaps the PS3 that uses the classic sparse addressing, so the odds of such a system also using gigantic hugetlbfs pages that it wants to demote is likely quite small. Thanks, -Doug
On 9/15/22 02:48, Mike Kravetz wrote: > On 09/14/22 12:09, Doug Berger wrote: >> With gigantic pages it may not be true that struct page structures >> are contiguous across the entire gigantic page. The nth_page macro >> is used here in place of direct pointer arithmetic to correct for >> this. >> >> Fixes: 8531fc6f52f5 ("hugetlb: add hugetlb demote page support") >> Signed-off-by: Doug Berger <opendmb@gmail.com> >> Cc: <stable@vger.kernel.org> >> --- >> mm/hugetlb.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) > > Thanks! > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > To answer Andrew's question about user-visible runtime effects. > We could get addressing exceptions. However, this is only possible in > configurations where CONFIG_SPARSEMEM && !CONFIG_SPARSEMEM_VMEMMAP. > Such a configuration option is rare an unknown to be the default > anywhere. In that case, should this be a 'Cc: stable' ? Although it does fix the above mentioned commit for a possible configuration. But should this be backported, if there could not have been an affected system ?
On 9/15/22 00:39, Doug Berger wrote: > With gigantic pages it may not be true that struct page structures > are contiguous across the entire gigantic page. The nth_page macro > is used here in place of direct pointer arithmetic to correct for > this. > > Fixes: 8531fc6f52f5 ("hugetlb: add hugetlb demote page support") > Signed-off-by: Doug Berger <opendmb@gmail.com> > Cc: <stable@vger.kernel.org> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > mm/hugetlb.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index e070b8593b37..0bdfc7e1c933 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -3420,6 +3420,7 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) > { > int i, nid = page_to_nid(page); > struct hstate *target_hstate; > + struct page *subpage; > int rc = 0; > > target_hstate = size_to_hstate(PAGE_SIZE << h->demote_order); > @@ -3453,15 +3454,16 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) > mutex_lock(&target_hstate->resize_lock); > for (i = 0; i < pages_per_huge_page(h); > i += pages_per_huge_page(target_hstate)) { > + subpage = nth_page(page, i); > if (hstate_is_gigantic(target_hstate)) > - prep_compound_gigantic_page_for_demote(page + i, > + prep_compound_gigantic_page_for_demote(subpage, > target_hstate->order); > else > - prep_compound_page(page + i, target_hstate->order); > - set_page_private(page + i, 0); > - set_page_refcounted(page + i); > - prep_new_huge_page(target_hstate, page + i, nid); > - put_page(page + i); > + prep_compound_page(subpage, target_hstate->order); > + set_page_private(subpage, 0); > + set_page_refcounted(subpage); > + prep_new_huge_page(target_hstate, subpage, nid); > + put_page(subpage); > } > mutex_unlock(&target_hstate->resize_lock); >
On Wed, Sep 14, 2022 at 12:09:17PM -0700, Doug Berger wrote: > With gigantic pages it may not be true that struct page structures > are contiguous across the entire gigantic page. The nth_page macro > is used here in place of direct pointer arithmetic to correct for > this. > > Fixes: 8531fc6f52f5 ("hugetlb: add hugetlb demote page support") > Signed-off-by: Doug Berger <opendmb@gmail.com> > Cc: <stable@vger.kernel.org> Reviewed-by: Oscar Salvador <osalvador@suse.de>
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index e070b8593b37..0bdfc7e1c933 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -3420,6 +3420,7 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) { int i, nid = page_to_nid(page); struct hstate *target_hstate; + struct page *subpage; int rc = 0; target_hstate = size_to_hstate(PAGE_SIZE << h->demote_order); @@ -3453,15 +3454,16 @@ static int demote_free_huge_page(struct hstate *h, struct page *page) mutex_lock(&target_hstate->resize_lock); for (i = 0; i < pages_per_huge_page(h); i += pages_per_huge_page(target_hstate)) { + subpage = nth_page(page, i); if (hstate_is_gigantic(target_hstate)) - prep_compound_gigantic_page_for_demote(page + i, + prep_compound_gigantic_page_for_demote(subpage, target_hstate->order); else - prep_compound_page(page + i, target_hstate->order); - set_page_private(page + i, 0); - set_page_refcounted(page + i); - prep_new_huge_page(target_hstate, page + i, nid); - put_page(page + i); + prep_compound_page(subpage, target_hstate->order); + set_page_private(subpage, 0); + set_page_refcounted(subpage); + prep_new_huge_page(target_hstate, subpage, nid); + put_page(subpage); } mutex_unlock(&target_hstate->resize_lock);
With gigantic pages it may not be true that struct page structures are contiguous across the entire gigantic page. The nth_page macro is used here in place of direct pointer arithmetic to correct for this. Fixes: 8531fc6f52f5 ("hugetlb: add hugetlb demote page support") Signed-off-by: Doug Berger <opendmb@gmail.com> Cc: <stable@vger.kernel.org> --- mm/hugetlb.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)