| Message ID | 20210322204836.1650221-1-axelrasmussen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | userfaultfd/shmem: fix minor fault page leak | expand |
On Mon, Mar 22, 2021 at 01:48:35PM -0700, Axel Rasmussen wrote: > This fix is analogous to Peter Xu's fix for hugetlb [0]. If we don't > put_page() after getting the page out of the page cache, we leak the > reference. > > The fix can be verified by checking /proc/meminfo and running the > userfaultfd selftest in shmem mode. Without the fix, we see MemFree / > MemAvailable steadily decreasing with each run of the test. With the > fix, memory is correctly freed after the test program exits. > > Fixes: 00da60b9d0a0 ("userfaultfd: support minor fault handling for shmem") > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> Reviewed-by: Peter Xu <peterx@redhat.com>
On Mon, 22 Mar 2021 13:48:35 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: > This fix is analogous to Peter Xu's fix for hugetlb [0]. If we don't > put_page() after getting the page out of the page cache, we leak the > reference. > > The fix can be verified by checking /proc/meminfo and running the > userfaultfd selftest in shmem mode. Without the fix, we see MemFree / > MemAvailable steadily decreasing with each run of the test. With the > fix, memory is correctly freed after the test program exits. > > Fixes: 00da60b9d0a0 ("userfaultfd: support minor fault handling for shmem") Confused. The affected code: > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -1831,6 +1831,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index, > > if (page && vma && userfaultfd_minor(vma)) { > unlock_page(page); > + put_page(page); > *fault_type = handle_userfault(vmf, VM_UFFD_MINOR); > return 0; > } Is added by Peter's "page && vma && userfaultfd_minor". I assume that "Fixes:" is incorrect?
Hi, Andrew, On Wed, Mar 24, 2021 at 04:20:27PM -0700, Andrew Morton wrote: > On Mon, 22 Mar 2021 13:48:35 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: > > > This fix is analogous to Peter Xu's fix for hugetlb [0]. If we don't > > put_page() after getting the page out of the page cache, we leak the > > reference. > > > > The fix can be verified by checking /proc/meminfo and running the > > userfaultfd selftest in shmem mode. Without the fix, we see MemFree / > > MemAvailable steadily decreasing with each run of the test. With the > > fix, memory is correctly freed after the test program exits. > > > > Fixes: 00da60b9d0a0 ("userfaultfd: support minor fault handling for shmem") > > Confused. The affected code: > > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -1831,6 +1831,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index, > > > > if (page && vma && userfaultfd_minor(vma)) { > > unlock_page(page); > > + put_page(page); > > *fault_type = handle_userfault(vmf, VM_UFFD_MINOR); > > return 0; > > } > > Is added by Peter's "page && vma && userfaultfd_minor". I assume that > "Fixes:" is incorrect? > It seems to me the commit is correct as pointed to in "Fixes", but I do have a different commit ID here: commit 63c826b1372c4930f89b8a55092699fa7f0d6f4e Author: Axel Rasmussen <axelrasmussen@google.com> Date: Thu Mar 18 10:20:43 2021 -0400 userfaultfd: support minor fault handling for shmem Axel, did you fetched the commit ID from your local tree, perhaps? Since I should have fetched from hnaz/linux-mm and I can see Andrew's sign-off too. Thanks,
On Wed, Mar 24, 2021 at 5:52 PM Peter Xu <peterx@redhat.com> wrote: > > Hi, Andrew, > > On Wed, Mar 24, 2021 at 04:20:27PM -0700, Andrew Morton wrote: > > On Mon, 22 Mar 2021 13:48:35 -0700 Axel Rasmussen <axelrasmussen@google.com> wrote: > > > > > This fix is analogous to Peter Xu's fix for hugetlb [0]. If we don't > > > put_page() after getting the page out of the page cache, we leak the > > > reference. > > > > > > The fix can be verified by checking /proc/meminfo and running the > > > userfaultfd selftest in shmem mode. Without the fix, we see MemFree / > > > MemAvailable steadily decreasing with each run of the test. With the > > > fix, memory is correctly freed after the test program exits. > > > > > > Fixes: 00da60b9d0a0 ("userfaultfd: support minor fault handling for shmem") > > > > Confused. The affected code: > > > > > --- a/mm/shmem.c > > > +++ b/mm/shmem.c > > > @@ -1831,6 +1831,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index, > > > > > > if (page && vma && userfaultfd_minor(vma)) { > > > unlock_page(page); > > > + put_page(page); > > > *fault_type = handle_userfault(vmf, VM_UFFD_MINOR); > > > return 0; > > > } > > > > Is added by Peter's "page && vma && userfaultfd_minor". I assume that > > "Fixes:" is incorrect? > > > > It seems to me the commit is correct as pointed to in "Fixes", but I do have a > different commit ID here: > > commit 63c826b1372c4930f89b8a55092699fa7f0d6f4e > Author: Axel Rasmussen <axelrasmussen@google.com> > Date: Thu Mar 18 10:20:43 2021 -0400 > > userfaultfd: support minor fault handling for shmem > > Axel, did you fetched the commit ID from your local tree, perhaps? Since I > should have fetched from hnaz/linux-mm and I can see Andrew's sign-off too. > > Thanks, > > -- > Peter Xu > Ah, this is the SHA I see when I "git log --grep linux-next/akpm" (where my repo's linux-next remote is [1]): commit 00da60b9d0a03818c36a2fe862578309c27006ad Author: Axel Rasmussen <axelrasmussen@google.com> Date: Thu Mar 18 17:01:51 2021 +1100 userfaultfd: support minor fault handling for shmem This is the commit that this new patch fixes. I'll admit I'm a bit unsure which tree the "Fixes:" tag is meant to refer to before the commits make it into Linus' tree, if I should look up the commit another way just let me know. :) And, sorry for the confusion. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
diff --git a/mm/shmem.c b/mm/shmem.c index ef8c9f5e92fc..d2e0e81b7d2e 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1831,6 +1831,7 @@ static int shmem_getpage_gfp(struct inode *inode, pgoff_t index, if (page && vma && userfaultfd_minor(vma)) { unlock_page(page); + put_page(page); *fault_type = handle_userfault(vmf, VM_UFFD_MINOR); return 0; }
This fix is analogous to Peter Xu's fix for hugetlb [0]. If we don't put_page() after getting the page out of the page cache, we leak the reference. The fix can be verified by checking /proc/meminfo and running the userfaultfd selftest in shmem mode. Without the fix, we see MemFree / MemAvailable steadily decreasing with each run of the test. With the fix, memory is correctly freed after the test program exits. Fixes: 00da60b9d0a0 ("userfaultfd: support minor fault handling for shmem") Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> --- mm/shmem.c | 1 + 1 file changed, 1 insertion(+)