On Fri, Dec 13, 2019 at 4:23 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 13 Dec 2019 13:40:15 -0800 John Hubbard <jhubbard@xxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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.
