| Message ID | 20210923122642.4999-1-zhengqi.arch@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memory_failure: Fix the missing pte_unmap() call | expand |
On 23.09.21 14:26, Qi Zheng wrote: > The paired pte_unmap() call is missing before the > dev_pagemap_mapping_shift() returns. So fix it. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > mm/memory-failure.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index e2984c123e7e..4e5419f16fd4 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -306,6 +306,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, > struct vm_area_struct *vma) > { > unsigned long address = vma_address(page, vma); > + unsigned long ret = 0; > pgd_t *pgd; > p4d_t *p4d; > pud_t *pud; > @@ -330,10 +331,12 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, > return PMD_SHIFT; > pte = pte_offset_map(pmd, address); > if (!pte_present(*pte)) > - return 0; > + goto unmap; > if (pte_devmap(*pte)) > - return PAGE_SHIFT; > - return 0; > + ret = PAGE_SHIFT; > +unmap: > + pte_unmap(pte); > + return ret; > } > > /* > I guess this code never runs on 32bit / highmem, that's why we didn't notice so far. Reviewed-by: David Hildenbrand <david@redhat.com>
On 9/23/21 11:19 PM, David Hildenbrand wrote: > On 23.09.21 14:26, Qi Zheng wrote: >> The paired pte_unmap() call is missing before the >> dev_pagemap_mapping_shift() returns. So fix it. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> mm/memory-failure.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index e2984c123e7e..4e5419f16fd4 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -306,6 +306,7 @@ static unsigned long >> dev_pagemap_mapping_shift(struct page *page, >> struct vm_area_struct *vma) >> { >> unsigned long address = vma_address(page, vma); >> + unsigned long ret = 0; >> pgd_t *pgd; >> p4d_t *p4d; >> pud_t *pud; >> @@ -330,10 +331,12 @@ static unsigned long >> dev_pagemap_mapping_shift(struct page *page, >> return PMD_SHIFT; >> pte = pte_offset_map(pmd, address); >> if (!pte_present(*pte)) >> - return 0; >> + goto unmap; >> if (pte_devmap(*pte)) >> - return PAGE_SHIFT; >> - return 0; >> + ret = PAGE_SHIFT; >> +unmap: >> + pte_unmap(pte); >> + return ret; >> } >> /* >> > > I guess this code never runs on 32bit / highmem, that's why we didn't > notice so far. I guess so too. > > Reviewed-by: David Hildenbrand <david@redhat.com> > Thanks, Qi
On Thu, 23 Sep 2021 20:26:42 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote: > The paired pte_unmap() call is missing before the > dev_pagemap_mapping_shift() returns. So fix it. > > ... > > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -306,6 +306,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, > struct vm_area_struct *vma) > { > unsigned long address = vma_address(page, vma); > + unsigned long ret = 0; > pgd_t *pgd; > p4d_t *p4d; > pud_t *pud; > @@ -330,10 +331,12 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, > return PMD_SHIFT; > pte = pte_offset_map(pmd, address); > if (!pte_present(*pte)) > - return 0; > + goto unmap; > if (pte_devmap(*pte)) > - return PAGE_SHIFT; > - return 0; > + ret = PAGE_SHIFT; > +unmap: > + pte_unmap(pte); > + return ret; > } > This is neater? +++ a/mm/memory-failure.c @@ -330,11 +330,8 @@ static unsigned long dev_pagemap_mapping if (pmd_devmap(*pmd)) return PMD_SHIFT; pte = pte_offset_map(pmd, address); - if (!pte_present(*pte)) - goto unmap; - if (pte_devmap(*pte)) + if (pte_present(*pte) && pte_devmap(*pte)) ret = PAGE_SHIFT; -unmap: pte_unmap(pte); return ret; }
On Thu, Sep 23, 2021 at 04:17:38PM -0700, Andrew Morton wrote: > On Thu, 23 Sep 2021 20:26:42 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > > The paired pte_unmap() call is missing before the > > dev_pagemap_mapping_shift() returns. So fix it. > > > > ... > > > > --- a/mm/memory-failure.c > > +++ b/mm/memory-failure.c > > @@ -306,6 +306,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, > > struct vm_area_struct *vma) > > { > > unsigned long address = vma_address(page, vma); > > + unsigned long ret = 0; > > pgd_t *pgd; > > p4d_t *p4d; > > pud_t *pud; > > @@ -330,10 +331,12 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, > > return PMD_SHIFT; > > pte = pte_offset_map(pmd, address); > > if (!pte_present(*pte)) > > - return 0; > > + goto unmap; > > if (pte_devmap(*pte)) > > - return PAGE_SHIFT; > > - return 0; > > + ret = PAGE_SHIFT; > > +unmap: > > + pte_unmap(pte); > > + return ret; > > } > > > > This is neater? > > +++ a/mm/memory-failure.c > @@ -330,11 +330,8 @@ static unsigned long dev_pagemap_mapping > if (pmd_devmap(*pmd)) > return PMD_SHIFT; > pte = pte_offset_map(pmd, address); > - if (!pte_present(*pte)) > - goto unmap; > - if (pte_devmap(*pte)) > + if (pte_present(*pte) && pte_devmap(*pte)) > ret = PAGE_SHIFT; > -unmap: This neater one looks good to me. Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
On 9/24/21 7:17 AM, Andrew Morton wrote: > On Thu, 23 Sep 2021 20:26:42 +0800 Qi Zheng <zhengqi.arch@bytedance.com> wrote: > >> The paired pte_unmap() call is missing before the >> dev_pagemap_mapping_shift() returns. So fix it. >> >> ... >> >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -306,6 +306,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, >> struct vm_area_struct *vma) >> { >> unsigned long address = vma_address(page, vma); >> + unsigned long ret = 0; >> pgd_t *pgd; >> p4d_t *p4d; >> pud_t *pud; >> @@ -330,10 +331,12 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, >> return PMD_SHIFT; >> pte = pte_offset_map(pmd, address); >> if (!pte_present(*pte)) >> - return 0; >> + goto unmap; >> if (pte_devmap(*pte)) >> - return PAGE_SHIFT; >> - return 0; >> + ret = PAGE_SHIFT; >> +unmap: >> + pte_unmap(pte); >> + return ret; >> } >> > > This is neater? Yes, and I see this neater version has merged into your mm tree, thanks. :) > > +++ a/mm/memory-failure.c > @@ -330,11 +330,8 @@ static unsigned long dev_pagemap_mapping > if (pmd_devmap(*pmd)) > return PMD_SHIFT; > pte = pte_offset_map(pmd, address); > - if (!pte_present(*pte)) > - goto unmap; > - if (pte_devmap(*pte)) > + if (pte_present(*pte) && pte_devmap(*pte)) > ret = PAGE_SHIFT; > -unmap: > pte_unmap(pte); > return ret; > } > _ >
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index e2984c123e7e..4e5419f16fd4 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -306,6 +306,7 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, struct vm_area_struct *vma) { unsigned long address = vma_address(page, vma); + unsigned long ret = 0; pgd_t *pgd; p4d_t *p4d; pud_t *pud; @@ -330,10 +331,12 @@ static unsigned long dev_pagemap_mapping_shift(struct page *page, return PMD_SHIFT; pte = pte_offset_map(pmd, address); if (!pte_present(*pte)) - return 0; + goto unmap; if (pte_devmap(*pte)) - return PAGE_SHIFT; - return 0; + ret = PAGE_SHIFT; +unmap: + pte_unmap(pte); + return ret; } /*
The paired pte_unmap() call is missing before the dev_pagemap_mapping_shift() returns. So fix it. Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- mm/memory-failure.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)