| Message ID | 20191211174653.4102-1-navid.emamdoost@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: Fix memory leak in __gup_benchmark_ioctl | expand |
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 >
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,
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,
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, > _ >
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; }
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(+)