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