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.
--- 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,
_
