[PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Pingfan Liu]

As for FOLL_LONGTERM, it is checked in the slow path
__gup_longterm_unlocked(). But it is not checked in the fast path, which
means a possible leak of CMA page to longterm pinned requirement through
this crack.

Place a check in the fast path.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: linux-kernel@vger.kernel.org
---
 mm/gup.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index f173fcb..6fe2feb 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2196,6 +2196,29 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,
 	return ret;
 }
 
+#if defined(CONFIG_CMA)
+static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
+	struct page **pages)
+{
+	if (unlikely(gup_flags & FOLL_LONGTERM)) {
+		int i = 0;
+
+		for (i = 0; i < nr_pinned; i++)
+			if (is_migrate_cma_page(pages[i])) {
+				put_user_pages(pages + i, nr_pinned - i);
+				return i;
+			}
+	}
+	return nr_pinned;
+}
+#else
+static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
+	struct page **pages)
+{
+	return nr_pinned;
+}
+#endif
+
 /**
  * get_user_pages_fast() - pin user pages in memory
  * @start:	starting user address
@@ -2236,6 +2259,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
 		ret = nr;
 	}
 
+	nr = reject_cma_pages(nr, gup_flags, pages);
 	if (nr < nr_pages) {
 		/* Try to get the remaining pages with get_user_pages */
 		start += nr << PAGE_SHIFT;
-- 
2.7.5

[PATCHv2 2/2] mm/gup: rename nr as nr_pinned in get_user_pages_fast()

[Pingfan Liu]

To better reflect the held state of pages and make code self-explaining,
rename nr as nr_pinned.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: linux-kernel@vger.kernel.org
---
 mm/gup.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 6fe2feb..106ab22 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2239,7 +2239,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
 			unsigned int gup_flags, struct page **pages)
 {
 	unsigned long addr, len, end;
-	int nr = 0, ret = 0;
+	int nr_pinned = 0, ret = 0;
 
 	start &= PAGE_MASK;
 	addr = start;
@@ -2254,26 +2254,26 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
 
 	if (gup_fast_permitted(start, nr_pages)) {
 		local_irq_disable();
-		gup_pgd_range(addr, end, gup_flags, pages, &nr);
+		gup_pgd_range(addr, end, gup_flags, pages, &nr_pinned);
 		local_irq_enable();
-		ret = nr;
+		ret = nr_pinned;
 	}
 
-	nr = reject_cma_pages(nr, gup_flags, pages);
-	if (nr < nr_pages) {
+	nr_pinned = reject_cma_pages(nr_pinned, gup_flags, pages);
+	if (nr_pinned < nr_pages) {
 		/* Try to get the remaining pages with get_user_pages */
-		start += nr << PAGE_SHIFT;
-		pages += nr;
+		start += nr_pinned << PAGE_SHIFT;
+		pages += nr_pinned;
 
-		ret = __gup_longterm_unlocked(start, nr_pages - nr,
+		ret = __gup_longterm_unlocked(start, nr_pages - nr_pinned,
 					      gup_flags, pages);
 
 		/* Have to be a bit careful with return values */
-		if (nr > 0) {
+		if (nr_pinned > 0) {
 			if (ret < 0)
-				ret = nr;
+				ret = nr_pinned;
 			else
-				ret += nr;
+				ret += nr_pinned;
 		}
 	}
 
-- 
2.7.5

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Ira Weiny]

On Mon, Jun 03, 2019 at 02:34:12PM +0800, Pingfan Liu wrote:
> As for FOLL_LONGTERM, it is checked in the slow path
> __gup_longterm_unlocked(). But it is not checked in the fast path, which
> means a possible leak of CMA page to longterm pinned requirement through
> this crack.
> 
> Place a check in the fast path.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>

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

> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/gup.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index f173fcb..6fe2feb 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2196,6 +2196,29 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,
>  	return ret;
>  }
>  
> +#if defined(CONFIG_CMA)
> +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> +	struct page **pages)
> +{
> +	if (unlikely(gup_flags & FOLL_LONGTERM)) {
> +		int i = 0;
> +
> +		for (i = 0; i < nr_pinned; i++)
> +			if (is_migrate_cma_page(pages[i])) {
> +				put_user_pages(pages + i, nr_pinned - i);
> +				return i;
> +			}
> +	}
> +	return nr_pinned;
> +}
> +#else
> +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> +	struct page **pages)
> +{
> +	return nr_pinned;
> +}
> +#endif
> +
>  /**
>   * get_user_pages_fast() - pin user pages in memory
>   * @start:	starting user address
> @@ -2236,6 +2259,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>  		ret = nr;
>  	}
>  
> +	nr = reject_cma_pages(nr, gup_flags, pages);
>  	if (nr < nr_pages) {
>  		/* Try to get the remaining pages with get_user_pages */
>  		start += nr << PAGE_SHIFT;
> -- 
> 2.7.5
> 

Re: [PATCHv2 2/2] mm/gup: rename nr as nr_pinned in get_user_pages_fast()

[Ira Weiny]

On Mon, Jun 03, 2019 at 02:34:13PM +0800, Pingfan Liu wrote:
> To better reflect the held state of pages and make code self-explaining,
> rename nr as nr_pinned.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>

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

> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/gup.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 6fe2feb..106ab22 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2239,7 +2239,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>  			unsigned int gup_flags, struct page **pages)
>  {
>  	unsigned long addr, len, end;
> -	int nr = 0, ret = 0;
> +	int nr_pinned = 0, ret = 0;
>  
>  	start &= PAGE_MASK;
>  	addr = start;
> @@ -2254,26 +2254,26 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>  
>  	if (gup_fast_permitted(start, nr_pages)) {
>  		local_irq_disable();
> -		gup_pgd_range(addr, end, gup_flags, pages, &nr);
> +		gup_pgd_range(addr, end, gup_flags, pages, &nr_pinned);
>  		local_irq_enable();
> -		ret = nr;
> +		ret = nr_pinned;
>  	}
>  
> -	nr = reject_cma_pages(nr, gup_flags, pages);
> -	if (nr < nr_pages) {
> +	nr_pinned = reject_cma_pages(nr_pinned, gup_flags, pages);
> +	if (nr_pinned < nr_pages) {
>  		/* Try to get the remaining pages with get_user_pages */
> -		start += nr << PAGE_SHIFT;
> -		pages += nr;
> +		start += nr_pinned << PAGE_SHIFT;
> +		pages += nr_pinned;
>  
> -		ret = __gup_longterm_unlocked(start, nr_pages - nr,
> +		ret = __gup_longterm_unlocked(start, nr_pages - nr_pinned,
>  					      gup_flags, pages);
>  
>  		/* Have to be a bit careful with return values */
> -		if (nr > 0) {
> +		if (nr_pinned > 0) {
>  			if (ret < 0)
> -				ret = nr;
> +				ret = nr_pinned;
>  			else
> -				ret += nr;
> +				ret += nr_pinned;
>  		}
>  	}
>  
> -- 
> 2.7.5
> 

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Christoph Hellwig]

> +#if defined(CONFIG_CMA)

You can just use #ifdef here.

> +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> +	struct page **pages)

Please use two instead of one tab to indent the continuing line of
a function declaration.

> +{
> +	if (unlikely(gup_flags & FOLL_LONGTERM)) {

IMHO it would be a little nicer if we could move this into the caller.

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Ira Weiny]

On Mon, Jun 03, 2019 at 09:42:06AM -0700, Christoph Hellwig wrote:
> > +#if defined(CONFIG_CMA)
> 
> You can just use #ifdef here.
> 
> > +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> > +	struct page **pages)
> 
> Please use two instead of one tab to indent the continuing line of
> a function declaration.
> 
> > +{
> > +	if (unlikely(gup_flags & FOLL_LONGTERM)) {
> 
> IMHO it would be a little nicer if we could move this into the caller.

FWIW we already had this discussion and thought it better to put this here.

https://lkml.org/lkml/2019/5/30/1565

Ira

[PS John for some reason your responses don't appear in that thread?]

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Christoph Hellwig]

On Mon, Jun 03, 2019 at 04:56:10PM -0700, Ira Weiny wrote:
> On Mon, Jun 03, 2019 at 09:42:06AM -0700, Christoph Hellwig wrote:
> > > +#if defined(CONFIG_CMA)
> > 
> > You can just use #ifdef here.
> > 
> > > +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> > > +	struct page **pages)
> > 
> > Please use two instead of one tab to indent the continuing line of
> > a function declaration.
> > 
> > > +{
> > > +	if (unlikely(gup_flags & FOLL_LONGTERM)) {
> > 
> > IMHO it would be a little nicer if we could move this into the caller.
> 
> FWIW we already had this discussion and thought it better to put this here.
> 
> https://lkml.org/lkml/2019/5/30/1565

I don't see any discussion like this.  FYI, this is what I mean,
code might be easier than words:


diff --git a/mm/gup.c b/mm/gup.c
index ddde097cf9e4..62d770b18e2c 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2197,6 +2197,27 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,
 	return ret;
 }
 
+#ifdef CONFIG_CMA
+static int reject_cma_pages(struct page **pages, int nr_pinned)
+{
+	int i = 0;
+
+	for (i = 0; i < nr_pinned; i++)
+		if (is_migrate_cma_page(pages[i])) {
+			put_user_pages(pages + i, nr_pinned - i);
+			return i;
+		}
+	}
+
+	return nr_pinned;
+}
+#else
+static inline int reject_cma_pages(struct page **pages, int nr_pinned)
+{
+	return nr_pinned;
+}
+#endif /* CONFIG_CMA */
+
 /**
  * get_user_pages_fast() - pin user pages in memory
  * @start:	starting user address
@@ -2237,6 +2258,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
 		ret = nr;
 	}
 
+	if (nr && unlikely(gup_flags & FOLL_LONGTERM))
+		nr = reject_cma_pages(pages, nr);
+
 	if (nr < nr_pages) {
 		/* Try to get the remaining pages with get_user_pages */
 		start += nr << PAGE_SHIFT;

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Pingfan Liu]

On Tue, Jun 4, 2019 at 12:42 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +#if defined(CONFIG_CMA)
>
> You can just use #ifdef here.
>
OK.
> > +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> > +     struct page **pages)
>
> Please use two instead of one tab to indent the continuing line of
> a function declaration.
>
Is it a convention? scripts/checkpatch.pl can not detect it. Could you
show me some light so later I can avoid it?

Thanks for your review.

Regards,
  Pingfan
> > +{
> > +     if (unlikely(gup_flags & FOLL_LONGTERM)) {
>
> IMHO it would be a little nicer if we could move this into the caller.

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Christoph Hellwig]

On Tue, Jun 04, 2019 at 03:13:21PM +0800, Pingfan Liu wrote:
> Is it a convention? scripts/checkpatch.pl can not detect it. Could you
> show me some light so later I can avoid it?

If you look at most kernel code you can see two conventions:

 - double tabe indent
 - indent to the start of the first agument line

Everything else is rather unusual.

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Pingfan Liu]

On Tue, Jun 4, 2019 at 3:17 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jun 04, 2019 at 03:13:21PM +0800, Pingfan Liu wrote:
> > Is it a convention? scripts/checkpatch.pl can not detect it. Could you
> > show me some light so later I can avoid it?
>
> If you look at most kernel code you can see two conventions:
>
>  - double tabe indent
>  - indent to the start of the first agument line
>
> Everything else is rather unusual.
OK.
Thanks

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Pingfan Liu]

On Tue, Jun 4, 2019 at 3:08 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jun 03, 2019 at 04:56:10PM -0700, Ira Weiny wrote:
> > On Mon, Jun 03, 2019 at 09:42:06AM -0700, Christoph Hellwig wrote:
> > > > +#if defined(CONFIG_CMA)
> > >
> > > You can just use #ifdef here.
> > >
> > > > +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> > > > + struct page **pages)
> > >
> > > Please use two instead of one tab to indent the continuing line of
> > > a function declaration.
> > >
> > > > +{
> > > > + if (unlikely(gup_flags & FOLL_LONGTERM)) {
> > >
> > > IMHO it would be a little nicer if we could move this into the caller.
> >
> > FWIW we already had this discussion and thought it better to put this here.
> >
> > https://lkml.org/lkml/2019/5/30/1565
>
> I don't see any discussion like this.  FYI, this is what I mean,
> code might be easier than words:
>
>
> diff --git a/mm/gup.c b/mm/gup.c
> index ddde097cf9e4..62d770b18e2c 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2197,6 +2197,27 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,
>         return ret;
>  }
>
> +#ifdef CONFIG_CMA
> +static int reject_cma_pages(struct page **pages, int nr_pinned)
> +{
> +       int i = 0;
> +
> +       for (i = 0; i < nr_pinned; i++)
> +               if (is_migrate_cma_page(pages[i])) {
> +                       put_user_pages(pages + i, nr_pinned - i);
> +                       return i;
> +               }
> +       }
> +
> +       return nr_pinned;
> +}
> +#else
> +static inline int reject_cma_pages(struct page **pages, int nr_pinned)
> +{
> +       return nr_pinned;
> +}
> +#endif /* CONFIG_CMA */
> +
>  /**
>   * get_user_pages_fast() - pin user pages in memory
>   * @start:     starting user address
> @@ -2237,6 +2258,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>                 ret = nr;
>         }
>
> +       if (nr && unlikely(gup_flags & FOLL_LONGTERM))
> +               nr = reject_cma_pages(pages, nr);
> +
Yeah. Looks better to keep reject_cma_pages() away from gup flags.

>         if (nr < nr_pages) {
>                 /* Try to get the remaining pages with get_user_pages */
>                 start += nr << PAGE_SHIFT;

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[Ira Weiny]

On Tue, Jun 04, 2019 at 12:08:08AM -0700, Christoph Hellwig wrote:
> On Mon, Jun 03, 2019 at 04:56:10PM -0700, Ira Weiny wrote:
> > On Mon, Jun 03, 2019 at 09:42:06AM -0700, Christoph Hellwig wrote:
> > > > +#if defined(CONFIG_CMA)
> > > 
> > > You can just use #ifdef here.
> > > 
> > > > +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
> > > > +	struct page **pages)
> > > 
> > > Please use two instead of one tab to indent the continuing line of
> > > a function declaration.
> > > 
> > > > +{
> > > > +	if (unlikely(gup_flags & FOLL_LONGTERM)) {
> > > 
> > > IMHO it would be a little nicer if we could move this into the caller.
> > 
> > FWIW we already had this discussion and thought it better to put this here.
> > 
> > https://lkml.org/lkml/2019/5/30/1565
> 
> I don't see any discussion like this.  FYI, this is what I mean,
> code might be easier than words:

Indeed that is more clear.  My apologies.

Ira

> 
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index ddde097cf9e4..62d770b18e2c 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2197,6 +2197,27 @@ static int __gup_longterm_unlocked(unsigned long start, int nr_pages,
>  	return ret;
>  }
>  
> +#ifdef CONFIG_CMA
> +static int reject_cma_pages(struct page **pages, int nr_pinned)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < nr_pinned; i++)
> +		if (is_migrate_cma_page(pages[i])) {
> +			put_user_pages(pages + i, nr_pinned - i);
> +			return i;
> +		}
> +	}
> +
> +	return nr_pinned;
> +}
> +#else
> +static inline int reject_cma_pages(struct page **pages, int nr_pinned)
> +{
> +	return nr_pinned;
> +}
> +#endif /* CONFIG_CMA */
> +
>  /**
>   * get_user_pages_fast() - pin user pages in memory
>   * @start:	starting user address
> @@ -2237,6 +2258,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>  		ret = nr;
>  	}
>  
> +	if (nr && unlikely(gup_flags & FOLL_LONGTERM))
> +		nr = reject_cma_pages(pages, nr);
> +
>  	if (nr < nr_pages) {
>  		/* Try to get the remaining pages with get_user_pages */
>  		start += nr << PAGE_SHIFT;
> 

Re: [PATCHv2 1/2] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

[John Hubbard]

On 6/3/19 4:56 PM, Ira Weiny wrote:
> On Mon, Jun 03, 2019 at 09:42:06AM -0700, Christoph Hellwig wrote:
>>> +#if defined(CONFIG_CMA)
>>
>> You can just use #ifdef here.
>>
>>> +static inline int reject_cma_pages(int nr_pinned, unsigned int gup_flags,
>>> +	struct page **pages)
>>
>> Please use two instead of one tab to indent the continuing line of
>> a function declaration.
>>
>>> +{
>>> +	if (unlikely(gup_flags & FOLL_LONGTERM)) {
>>
>> IMHO it would be a little nicer if we could move this into the caller.
> 
> FWIW we already had this discussion and thought it better to put this here.
> 
> https://lkml.org/lkml/2019/5/30/1565
> 
> Ira
> 
> [PS John for some reason your responses don't appear in that thread?]


Thanks for pointing out the email glitches! It looks like it's making it over to
lore.kernel.org/linux-mm, but not to lkml.org, nor to the lore.kernel.org/lkml 
section either:

    https://lore.kernel.org/linux-mm/e389551e-32c3-c9f2-2861-1a8819dc7cc9@nvidia.com/

...and I've already checked the DKIM signatures, they're all good. So I think this
is getting narrowed down to, messages from nvidia.com (or at least from me) are not
making it onto the lkml list server.  I'm told that this can actually happen *because*
of DKIM domains: list servers may try to avoid retransmitting from DKIM domains. sigh.

Any hints are welcome, otherwise I'll try to locate the lkml admins and see what can
be done.

(+Sanket, Ralph from our email team)


thanks,
-- 
John Hubbard
NVIDIA