On Wed, Sep 04, 2024 at 12:41:08PM -0700, Steve Sistare wrote:
> The folio_try_get in memfd_alloc_folio is not necessary. Delete it, and
> delete the matching folio_put in memfd_pin_folios. This also avoids
> leaking a ref if the memfd_alloc_folio call to hugetlb_add_to_page_cache
> fails, which would otherwise need an additional folio_put. This is a
> continuation of the fix
> "mm/hugetlb: fix memfd_pin_folios free_huge_pages leak"
I think you're right, but don't we also need to get rid of the
folio_put() call in the 'if (err)' case after calling
hugetlb_add_to_page_cache()?
> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning memfd folios")
>
> Suggested-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
> ---
> mm/gup.c | 4 +---
> mm/memfd.c | 2 +-
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index bccabaa..947881ff 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -3618,7 +3618,7 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
> pgoff_t start_idx, end_idx, next_idx;
> struct folio *folio = NULL;
> struct folio_batch fbatch;
> - struct hstate *h = NULL;
> + struct hstate *h;
> long ret = -EINVAL;
>
> if (start < 0 || start > end || !max_folios)
> @@ -3662,8 +3662,6 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
> &fbatch);
> if (folio) {
> folio_put(folio);
> - if (h)
> - folio_put(folio);
> folio = NULL;
> }
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index bcb131d..f715301 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -89,7 +89,7 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx)
> numa_node_id(),
> NULL,
> gfp_mask);
> - if (folio && folio_try_get(folio)) {
> + if (folio) {
> err = hugetlb_add_to_page_cache(folio,
> memfd->f_mapping,
> idx);
> --
> 1.8.3.1
>
