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

[Andrew Morton]

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