| Message ID | 20230701072837.1994253-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: memory-failure: fix potential page refcnt leak in memory_failure() | expand |
On Sat, Jul 01, 2023 at 03:28:37PM +0800, Miaohe Lin wrote: > put_ref_page() is not called to drop extra refcnt when comes from madvise > in the case pfn is valid but pgmap is NULL leading to page refcnt leak. Is this test scenario realistic one? I don't think that we can call madvise() for such a device memory page. If this is the case, this issue can be thought as potentioal one (so no need to send to stable). > > Fixes: 1e8aaedb182d ("mm,memory_failure: always pin the page in madvise_inject_error") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> Anyway, the patch looks good to me. Thanks you. Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com> > --- > mm/memory-failure.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index e245191e6b04..65e2d4c5b50d 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -2080,8 +2080,6 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags, > { > int rc = -ENXIO; > > - put_ref_page(pfn, flags); > - > /* device metadata space is not recoverable */ > if (!pgmap_pfn_valid(pgmap, pfn)) > goto out; > @@ -2157,6 +2155,7 @@ int memory_failure(unsigned long pfn, int flags) > > if (pfn_valid(pfn)) { > pgmap = get_dev_pagemap(pfn, NULL); > + put_ref_page(pfn, flags); > if (pgmap) { > res = memory_failure_dev_pagemap(pfn, flags, > pgmap); > -- > 2.33.0 > > >
On 2023/7/3 14:37, Naoya Horiguchi wrote: > On Sat, Jul 01, 2023 at 03:28:37PM +0800, Miaohe Lin wrote: >> put_ref_page() is not called to drop extra refcnt when comes from madvise >> in the case pfn is valid but pgmap is NULL leading to page refcnt leak. > > Is this test scenario realistic one? I don't think that we can call madvise() for > such a device memory page. If this is the case, this issue can be thought as > potentioal one (so no need to send to stable). This is a theoretical problem. If pagemap can ever be NULL after page refcnt is increased, this would occur. I think it's not needed to send to stable too. > >> >> Fixes: 1e8aaedb182d ("mm,memory_failure: always pin the page in madvise_inject_error") >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > > Anyway, the patch looks good to me. Thanks you. > > Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com> Thanks for your review and comment.
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index e245191e6b04..65e2d4c5b50d 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -2080,8 +2080,6 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags, { int rc = -ENXIO; - put_ref_page(pfn, flags); - /* device metadata space is not recoverable */ if (!pgmap_pfn_valid(pgmap, pfn)) goto out; @@ -2157,6 +2155,7 @@ int memory_failure(unsigned long pfn, int flags) if (pfn_valid(pfn)) { pgmap = get_dev_pagemap(pfn, NULL); + put_ref_page(pfn, flags); if (pgmap) { res = memory_failure_dev_pagemap(pfn, flags, pgmap);
put_ref_page() is not called to drop extra refcnt when comes from madvise in the case pfn is valid but pgmap is NULL leading to page refcnt leak. Fixes: 1e8aaedb182d ("mm,memory_failure: always pin the page in madvise_inject_error") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/memory-failure.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)