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

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

[Navid Emamdoost]

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;
 		}
 
-- 
2.17.1

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

[Ira Weiny]

On Wed, Dec 11, 2019 at 11:46:51AM -0600, 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);

I wonder if adding a ret value and a goto where the free is done would be
better.  But may be overkill at this time.  So...

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  			return -1;
>  		}
>  
> -- 
> 2.17.1
> 

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

[John Hubbard]

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. 

Either way, you can add:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA
 

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@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.

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

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

[Navid Emamdoost]

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.

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

[Navid Emamdoost]

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() by goto done.

Fixes: 714a3a1ebafe ("mm/gup_benchmark.c: add additional pinning methods")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v2:
	-- added goto and ret value instead of return -1.
---
 mm/gup_benchmark.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index b160638f647e..b773b2568544 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -24,7 +24,7 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
 {
 	ktime_t start_time, end_time;
 	unsigned long i, nr_pages, addr, next;
-	int nr;
+	int nr, ret = 0;
 	struct page **pages;
 
 	if (gup->size > ULONG_MAX)
@@ -63,8 +63,8 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
 					    NULL);
 			break;
 		default:
-			kvfree(pages);
-			return -1;
+			ret = -EINVAL;
+			goto done;
 		}
 
 		if (nr <= 0)
@@ -85,8 +85,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
 	end_time = ktime_get();
 	gup->put_delta_usec = ktime_us_delta(end_time, start_time);
 
+done:
 	kvfree(pages);
-	return 0;
+	return ret;
 }
 
 static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
-- 
2.17.1

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

[Ira Weiny]

On Fri, Dec 13, 2019 at 04:37:41PM -0600, 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() by goto done.
> 
> Fixes: 714a3a1ebafe ("mm/gup_benchmark.c: add additional pinning methods")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
> Changes in v2:
> 	-- added goto and ret value instead of return -1.
> ---
>  mm/gup_benchmark.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index b160638f647e..b773b2568544 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -24,7 +24,7 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  {
>  	ktime_t start_time, end_time;
>  	unsigned long i, nr_pages, addr, next;
> -	int nr;
> +	int nr, ret = 0;
>  	struct page **pages;
>  
>  	if (gup->size > ULONG_MAX)
> @@ -63,8 +63,8 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  					    NULL);
>  			break;
>  		default:
> -			kvfree(pages);
> -			return -1;
> +			ret = -EINVAL;
> +			goto done;
>  		}
>  
>  		if (nr <= 0)
> @@ -85,8 +85,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  	end_time = ktime_get();
>  	gup->put_delta_usec = ktime_us_delta(end_time, start_time);
>  
> +done:
>  	kvfree(pages);
> -	return 0;
> +	return ret;
>  }
>  
>  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
> -- 
> 2.17.1
> 
> 

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

[John Hubbard]

On 12/13/19 2:37 PM, 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() by goto done.
> 
> Fixes: 714a3a1ebafe ("mm/gup_benchmark.c: add additional pinning methods")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> 	-- added goto and ret value instead of return -1.
> ---
>  mm/gup_benchmark.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA

> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index b160638f647e..b773b2568544 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -24,7 +24,7 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  {
>  	ktime_t start_time, end_time;
>  	unsigned long i, nr_pages, addr, next;
> -	int nr;
> +	int nr, ret = 0;
>  	struct page **pages;
>  
>  	if (gup->size > ULONG_MAX)
> @@ -63,8 +63,8 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  					    NULL);
>  			break;
>  		default:
> -			kvfree(pages);
> -			return -1;
> +			ret = -EINVAL;
> +			goto done;
>  		}
>  
>  		if (nr <= 0)
> @@ -85,8 +85,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  	end_time = ktime_get();
>  	gup->put_delta_usec = ktime_us_delta(end_time, start_time);
>  
> +done:
>  	kvfree(pages);
> -	return 0;
> +	return ret;
>  }
>  
>  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
> 

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

[Markus Elfring]

…
> +++ b/mm/gup_benchmark.c
…
> @@ -85,8 +85,9 @@  static int __gup_benchmark_ioctl(unsigned int cmd,
>  	end_time = ktime_get();
>  	gup->put_delta_usec = ktime_us_delta(end_time, start_time);
>
> +done:
>  	kvfree(pages);
> -	return 0;
> +	return ret;
>  }
>
>  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,

Can the addition of a label like “free_pages” be more appropriate here?

Regards,
Markus

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

[David Hildenbrand]

On 13.12.19 23:37, 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() by goto done.
> 
> Fixes: 714a3a1ebafe ("mm/gup_benchmark.c: add additional pinning methods")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> 	-- added goto and ret value instead of return -1.
> ---
>  mm/gup_benchmark.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index b160638f647e..b773b2568544 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -24,7 +24,7 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  {
>  	ktime_t start_time, end_time;
>  	unsigned long i, nr_pages, addr, next;
> -	int nr;
> +	int nr, ret = 0;
>  	struct page **pages;
>  
>  	if (gup->size > ULONG_MAX)
> @@ -63,8 +63,8 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  					    NULL);
>  			break;
>  		default:
> -			kvfree(pages);
> -			return -1;
> +			ret = -EINVAL;
> +			goto done;
>  		}
>  
>  		if (nr <= 0)
> @@ -85,8 +85,9 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
>  	end_time = ktime_get();
>  	gup->put_delta_usec = ktime_us_delta(end_time, start_time);
>  
> +done:
>  	kvfree(pages);
> -	return 0;
> +	return ret;
>  }
>  
>  static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
> 

You should really CC people that give review feedback in previous
iterations (IOW me :) ) and properly tag versions/mention code change
since the last version.

(v1 was already <20191122224117.2372-1-navid.emamdoost@gmail.com>, this
here would be v3)

Also, this patch here is not a standalone patch, it is an addon on top
of <20191211174653.4102-1-navid.emamdoost@gmail.com>

Enough rambling :) This squashed into
<20191211174653.4102-1-navid.emamdoost@gmail.com>:

Reviewed-by: David Hildenbrand <david@redhat.com>


I agree that the label could have a better name (e.g., out_free)

-- 
Thanks,

David / dhildenb