| Message ID | 20210126031009.96266-1-songmuchun@bytedance.com |
|---|---|
| State | Accepted |
| Commit | e558464be982af2546229dcbef746d24e942abaa |
| Headers | show |
| Series | mm: hugetlb: fix missing put_page in gather_surplus_pages() | expand |
On 1/25/21 7:10 PM, Muchun Song wrote: > The VM_BUG_ON_PAGE avoids the generation of any code, even if that > expression has side-effects when !CONFIG_DEBUG_VM. > > Fixes: e5dfacebe4a4 ("mm/hugetlb.c: just use put_page_testzero() instead of page_count()") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Cc: <stable@vger.kernel.org> > --- > mm/hugetlb.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) Thanks for finding and fixing this! My bad for not noticing when the bug was introduced. > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index a6bad1f686c5..082ed643020b 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2047,13 +2047,16 @@ static int gather_surplus_pages(struct hstate *h, long delta) > > /* Free the needed pages to the hugetlb pool */ > list_for_each_entry_safe(page, tmp, &surplus_list, lru) { > + int zeroed; > + > if ((--needed) < 0) > break; > /* > * This page is now managed by the hugetlb allocator and has > * no users -- drop the buddy allocator's reference. > */ > - VM_BUG_ON_PAGE(!put_page_testzero(page), page); > + zeroed = put_page_testzero(page); > + VM_BUG_ON_PAGE(!zeroed, page); > enqueue_huge_page(h, page); I was wondering why this was not causing any problems. We are putting the hugetlb page on the free list with a count of 1. There is no check in the enqueue code. When we dequeue the page, set_page_refcounted() is used to set the count to 1 without looking at the current value. And, all the other VM_DEBUG macros are off so we mostly do not notice the bug. Thanks again, Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On Tue, Jan 26, 2021 at 12:31 PM Mike Kravetz <mike.kravetz@oracle.com> wrote: > > On 1/25/21 7:10 PM, Muchun Song wrote: > > The VM_BUG_ON_PAGE avoids the generation of any code, even if that > > expression has side-effects when !CONFIG_DEBUG_VM. > > > > Fixes: e5dfacebe4a4 ("mm/hugetlb.c: just use put_page_testzero() instead of page_count()") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Cc: <stable@vger.kernel.org> > > --- > > mm/hugetlb.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > Thanks for finding and fixing this! My bad for not noticing when the bug > was introduced. > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index a6bad1f686c5..082ed643020b 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -2047,13 +2047,16 @@ static int gather_surplus_pages(struct hstate *h, long delta) > > > > /* Free the needed pages to the hugetlb pool */ > > list_for_each_entry_safe(page, tmp, &surplus_list, lru) { > > + int zeroed; > > + > > if ((--needed) < 0) > > break; > > /* > > * This page is now managed by the hugetlb allocator and has > > * no users -- drop the buddy allocator's reference. > > */ > > - VM_BUG_ON_PAGE(!put_page_testzero(page), page); > > + zeroed = put_page_testzero(page); > > + VM_BUG_ON_PAGE(!zeroed, page); > > enqueue_huge_page(h, page); > > I was wondering why this was not causing any problems. We are putting the > hugetlb page on the free list with a count of 1. There is no check in the > enqueue code. When we dequeue the page, set_page_refcounted() is used to > set the count to 1 without looking at the current value. And, all the other > VM_DEBUG macros are off so we mostly do not notice the bug. Yeah. You are right. I also thought about this question. It is a very hidden bug. > > Thanks again, > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com> > > -- > Mike Kravetz > > > } > > free: > >
Hi: On 2021/1/26 11:10, Muchun Song wrote: > The VM_BUG_ON_PAGE avoids the generation of any code, even if that > expression has side-effects when !CONFIG_DEBUG_VM. > > Fixes: e5dfacebe4a4 ("mm/hugetlb.c: just use put_page_testzero() instead of page_count()") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > Cc: <stable@vger.kernel.org> > --- > mm/hugetlb.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index a6bad1f686c5..082ed643020b 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2047,13 +2047,16 @@ static int gather_surplus_pages(struct hstate *h, long delta) > > /* Free the needed pages to the hugetlb pool */ > list_for_each_entry_safe(page, tmp, &surplus_list, lru) { > + int zeroed; > + > if ((--needed) < 0) > break; > /* > * This page is now managed by the hugetlb allocator and has > * no users -- drop the buddy allocator's reference. > */ > - VM_BUG_ON_PAGE(!put_page_testzero(page), page); > + zeroed = put_page_testzero(page); > + VM_BUG_ON_PAGE(!zeroed, page); > enqueue_huge_page(h, page); > } > free: > Good Catch! Thanks. Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index a6bad1f686c5..082ed643020b 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2047,13 +2047,16 @@ static int gather_surplus_pages(struct hstate *h, long delta) /* Free the needed pages to the hugetlb pool */ list_for_each_entry_safe(page, tmp, &surplus_list, lru) { + int zeroed; + if ((--needed) < 0) break; /* * This page is now managed by the hugetlb allocator and has * no users -- drop the buddy allocator's reference. */ - VM_BUG_ON_PAGE(!put_page_testzero(page), page); + zeroed = put_page_testzero(page); + VM_BUG_ON_PAGE(!zeroed, page); enqueue_huge_page(h, page); } free:
The VM_BUG_ON_PAGE avoids the generation of any code, even if that expression has side-effects when !CONFIG_DEBUG_VM. Fixes: e5dfacebe4a4 ("mm/hugetlb.c: just use put_page_testzero() instead of page_count()") Signed-off-by: Muchun Song <songmuchun@bytedance.com> Cc: <stable@vger.kernel.org> --- mm/hugetlb.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)