Re: [PATCH] mm/gup: Fix memory leak in __gup_benchmark_ioctl

[Navid Emamdoost <navid.emamdoost@gmail.com>]

On Fri, Dec 13, 2019 at 4:23 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Dec 2019 13:40:15 -0800 John Hubbard <jhubbard@nvidia.com> wrote:
>
> > On 12/11/19 9:46 AM, Navid Emamdoost wrote:
> > > In the implementation of __gup_benchmark_ioctl() the allocated pages
> > > should be released before returning in case of an invalid cmd. Release
> > > pages via kvfree().
> > >
> > > Fixes: 714a3a1ebafe ("mm/gup_benchmark.c: add additional pinning methods")
> > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> > > ---
> > >  mm/gup_benchmark.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> > > index 7dd602d7f8db..b160638f647e 100644
> > > --- a/mm/gup_benchmark.c
> > > +++ b/mm/gup_benchmark.c
> > > @@ -63,6 +63,7 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> > >                                         NULL);
> > >                     break;
> > >             default:
> > > +                   kvfree(pages);
> > >                     return -1;
> > >             }
> > >
> >
> > Hi,
> >
> > The patch is correct, but I would like to second Ira's request for a ret value,
> > and a "goto done" to use a single place to kvfree, if you don't mind.
> >
>
> Fair enough.
>
> And let's make it return -EINVAL rather than -1, which appears to be
> -EPERM.

Sure! patch v2 has been sent.
>
> --- a/mm/gup_benchmark.c~mm-gup-fix-memory-leak-in-__gup_benchmark_ioctl-fix
> +++ a/mm/gup_benchmark.c
> @@ -26,6 +26,7 @@ static int __gup_benchmark_ioctl(unsigne
>         unsigned long i, nr_pages, addr, next;
>         int nr;
>         struct page **pages;
> +       int ret = 0;
>
>         if (gup->size > ULONG_MAX)
>                 return -EINVAL;
> @@ -64,7 +65,8 @@ static int __gup_benchmark_ioctl(unsigne
>                         break;
>                 default:
>                         kvfree(pages);
> -                       return -1;
> +                       ret = -EINVAL;
> +                       goto out;
>                 }
>
>                 if (nr <= 0)
> @@ -86,7 +88,8 @@ static int __gup_benchmark_ioctl(unsigne
>         gup->put_delta_usec = ktime_us_delta(end_time, start_time);
>
>         kvfree(pages);
> -       return 0;
> +out:
> +       return ret;
>  }
>
>  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
> _
>


-- 
Navid.