[bug report] hugetlb, mempolicy: fix the mbind hugetlb migration

[bug report] hugetlb, mempolicy: fix the mbind hugetlb migration

[Dan Carpenter]

Hello Michal Hocko,

This is a semi-automatic email about new static checker warnings.

The patch ef2fc869a863: "hugetlb, mempolicy: fix the mbind hugetlb 
migration" from Jan 5, 2018, leads to the following Smatch complaint:

    mm/mempolicy.c:1100 new_page()
    error: we previously assumed 'vma' could be null (see line 1092)

mm/mempolicy.c
  1091		vma = find_vma(current->mm, start);
  1092		while (vma) {
                       ^^^
There is a check for NULL here

  1093			address = page_address_in_vma(page, vma);
  1094			if (address != -EFAULT)
  1095				break;
  1096			vma = vma->vm_next;
  1097		}
  1098	
  1099		if (PageHuge(page)) {
  1100			return alloc_huge_page_vma(vma, address);
                                                   ^^^
The patch adds a new unchecked dereference.  It might be OK?  I don't
know.

  1101		} else if (PageTransHuge(page)) {
  1102			struct page *thp;

regards,
dan carpenter

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [bug report] hugetlb, mempolicy: fix the mbind hugetlb migration

[Michal Hocko]

[CC Mike and Naoya]

On Tue 09-01-18 23:05:39, Dan Carpenter wrote:
> Hello Michal Hocko,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch ef2fc869a863: "hugetlb, mempolicy: fix the mbind hugetlb 
> migration" from Jan 5, 2018, leads to the following Smatch complaint:
> 
>     mm/mempolicy.c:1100 new_page()
>     error: we previously assumed 'vma' could be null (see line 1092)
> 
> mm/mempolicy.c
>   1091		vma = find_vma(current->mm, start);
>   1092		while (vma) {
>                        ^^^
> There is a check for NULL here
> 
>   1093			address = page_address_in_vma(page, vma);
>   1094			if (address != -EFAULT)
>   1095				break;
>   1096			vma = vma->vm_next;
>   1097		}
>   1098	
>   1099		if (PageHuge(page)) {
>   1100			return alloc_huge_page_vma(vma, address);
>                                                    ^^^
> The patch adds a new unchecked dereference.  It might be OK?  I don't
> know.
> 
>   1101		} else if (PageTransHuge(page)) {
>   1102			struct page *thp;

Smatch is correct that the code is fishy. The patch you have outlined is
the last one to touch that area but it hasn't changed the vma logic.
It removed the BUG_ON which papepered over null VMA for your checker
previously I guess.

The THP path simply falls back to the default mem policy if vma is NULL.
We should do the same here. The patch below should do the trick.

Thanks for the report!

Re: [bug report] hugetlb, mempolicy: fix the mbind hugetlb migration

[Michal Hocko]

On Wed 10-01-18 11:47:12, Michal Hocko wrote:
> [CC Mike and Naoya]

ping

> From 7227218bd526cceb954a688727d78af0b5874e18 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Wed, 10 Jan 2018 11:40:20 +0100
> Subject: [PATCH] hugetlb, mbind: fall back to default policy if vma is NULL
> 
> Dan Carpenter has noticed that mbind migration callback (new_page)
> can get a NULL vma pointer and choke on it inside alloc_huge_page_vma
> which relies on the VMA to get the hstate. We used to BUG_ON this
> case but the BUG_+ON has been removed recently by "hugetlb, mempolicy:
> fix the mbind hugetlb migration".
> 
> The proper way to handle this is to get the hstate from the migrated
> page and rely on huge_node (resp. get_vma_policy) do the right thing
> with null VMA. We are currently falling back to the default mempolicy in
> that case which is in line what THP path is doing here.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/hugetlb.h | 5 +++--
>  mm/hugetlb.c            | 5 ++---
>  mm/mempolicy.c          | 3 ++-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 612a29b7f6c6..36fa6a2a82e3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -358,7 +358,8 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
>  struct page *alloc_huge_page_node(struct hstate *h, int nid);
>  struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
>  				nodemask_t *nmask);
> -struct page *alloc_huge_page_vma(struct vm_area_struct *vma, unsigned long address);
> +struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
> +				unsigned long address);
>  int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
>  			pgoff_t idx);
>  
> @@ -536,7 +537,7 @@ struct hstate {};
>  #define alloc_huge_page(v, a, r) NULL
>  #define alloc_huge_page_node(h, nid) NULL
>  #define alloc_huge_page_nodemask(h, preferred_nid, nmask) NULL
> -#define alloc_huge_page_vma(vma, address) NULL
> +#define alloc_huge_page_vma(h, vma, address) NULL
>  #define alloc_bootmem_huge_page(h) NULL
>  #define hstate_file(f) NULL
>  #define hstate_sizelog(s) NULL
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index ffcae114ceed..27872270ead7 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1675,16 +1675,15 @@ struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
>  }
>  
>  /* mempolicy aware migration callback */
> -struct page *alloc_huge_page_vma(struct vm_area_struct *vma, unsigned long address)
> +struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
> +		unsigned long address)
>  {
>  	struct mempolicy *mpol;
>  	nodemask_t *nodemask;
>  	struct page *page;
> -	struct hstate *h;
>  	gfp_t gfp_mask;
>  	int node;
>  
> -	h = hstate_vma(vma);
>  	gfp_mask = htlb_alloc_mask(h);
>  	node = huge_node(vma, address, gfp_mask, &mpol, &nodemask);
>  	page = alloc_huge_page_nodemask(h, node, nodemask);
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 30e68da64873..a8b7d59002e8 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1097,7 +1097,8 @@ static struct page *new_page(struct page *page, unsigned long start)
>  	}
>  
>  	if (PageHuge(page)) {
> -		return alloc_huge_page_vma(vma, address);
> +		return alloc_huge_page_vma(page_hstate(compound_head(page)),
> +				vma, address);
>  	} else if (PageTransHuge(page)) {
>  		struct page *thp;
>  
> -- 
> 2.15.1
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [bug report] hugetlb, mempolicy: fix the mbind hugetlb migration

[Naoya Horiguchi]

On 01/17/2018 09:18 PM, Michal Hocko wrote:
> On Wed 10-01-18 11:47:12, Michal Hocko wrote:
>> [CC Mike and Naoya]
> 
> ping
> 
>> From 7227218bd526cceb954a688727d78af0b5874e18 Mon Sep 17 00:00:00 2001
>> From: Michal Hocko <mhocko@suse.com>
>> Date: Wed, 10 Jan 2018 11:40:20 +0100
>> Subject: [PATCH] hugetlb, mbind: fall back to default policy if vma is NULL
>>
>> Dan Carpenter has noticed that mbind migration callback (new_page)
>> can get a NULL vma pointer and choke on it inside alloc_huge_page_vma
>> which relies on the VMA to get the hstate. We used to BUG_ON this
>> case but the BUG_+ON has been removed recently by "hugetlb, mempolicy:
>> fix the mbind hugetlb migration".
>>
>> The proper way to handle this is to get the hstate from the migrated
>> page and rely on huge_node (resp. get_vma_policy) do the right thing
>> with null VMA. We are currently falling back to the default mempolicy in
>> that case which is in line what THP path is doing here.

vma is used only for getting mempolicy in alloc_huge_page_vma(), so
falling back to default mempolicy looks better to me than BUG_ON.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks,
Naoya Horiguchi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [bug report] hugetlb, mempolicy: fix the mbind hugetlb migration

[Michal Hocko]

On Wed 17-01-18 23:15:03, Naoya Horiguchi wrote:
> On 01/17/2018 09:18 PM, Michal Hocko wrote:
> > On Wed 10-01-18 11:47:12, Michal Hocko wrote:
> >> [CC Mike and Naoya]
> > 
> > ping
> > 
> >> From 7227218bd526cceb954a688727d78af0b5874e18 Mon Sep 17 00:00:00 2001
> >> From: Michal Hocko <mhocko@suse.com>
> >> Date: Wed, 10 Jan 2018 11:40:20 +0100
> >> Subject: [PATCH] hugetlb, mbind: fall back to default policy if vma is NULL
> >>
> >> Dan Carpenter has noticed that mbind migration callback (new_page)
> >> can get a NULL vma pointer and choke on it inside alloc_huge_page_vma
> >> which relies on the VMA to get the hstate. We used to BUG_ON this
> >> case but the BUG_+ON has been removed recently by "hugetlb, mempolicy:
> >> fix the mbind hugetlb migration".
> >>
> >> The proper way to handle this is to get the hstate from the migrated
> >> page and rely on huge_node (resp. get_vma_policy) do the right thing
> >> with null VMA. We are currently falling back to the default mempolicy in
> >> that case which is in line what THP path is doing here.
> 
> vma is used only for getting mempolicy in alloc_huge_page_vma(), so
> falling back to default mempolicy looks better to me than BUG_ON.
> 
> Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks for the review Naoya!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>