Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 13 Dec 2019 14:23:32 -0800
From:   Andrew Morton <akpm@linux-foundation.org>
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     Navid Emamdoost <navid.emamdoost@gmail.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Keith Busch <keith.busch@intel.com>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, <emamd001@umn.edu>
Subject: Re: [PATCH] mm/gup: Fix memory leak in __gup_benchmark_ioctl
Message-Id: <20191213142332.d7fafc243291eac302375c32@linux-foundation.org>
In-Reply-To: <9a692d27-4654-f1fc-d4c5-c6efba02c8a9@nvidia.com>
References: <20191211174653.4102-1-navid.emamdoost@gmail.com>
        <9a692d27-4654-f1fc-d4c5-c6efba02c8a9@nvidia.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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