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;
MIME-Version: 1.0
References: <20191211174653.4102-1-navid.emamdoost@gmail.com>
 <9a692d27-4654-f1fc-d4c5-c6efba02c8a9@nvidia.com> <20191213142332.d7fafc243291eac302375c32@linux-foundation.org>
In-Reply-To: <20191213142332.d7fafc243291eac302375c32@linux-foundation.org>
From:   Navid Emamdoost <navid.emamdoost@gmail.com>
Date:   Fri, 13 Dec 2019 16:39:17 -0600
Message-ID: <CAEkB2EQ=sm-myT4jgJ+i0Cy5Gv=DH7vvixc1Dpz5V-Zt+P5oMQ@mail.gmail.com>
Subject: Re: [PATCH] mm/gup: Fix memory leak in __gup_benchmark_ioctl
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     John Hubbard <jhubbard@nvidia.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Keith Busch <keith.busch@intel.com>, linux-mm@kvack.org,
        LKML <linux-kernel@vger.kernel.org>,
        Navid Emamdoost <emamd001@umn.edu>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.