* [PATCH][mmotm] mbind: add BUG_ON(!vma) in new_vma_page() (was Re: mm: mbind: add hugepage migration code to mbind())
2013-08-19 6:54 mm: mbind: add hugepage migration code to mbind() Dan Carpenter
@ 2013-08-19 18:28 ` Naoya Horiguchi
0 siblings, 0 replies; 2+ messages in thread
From: Naoya Horiguchi @ 2013-08-19 18:28 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-mm, Andrew Morton, David Rientjes, KOSAKI Motohiro,
Lee Schermerhorn
Hi Dan,
(Cc:ed MM maintainers/developers.)
Thanks for reporting.
On Mon, Aug 19, 2013 at 09:54:50AM +0300, Dan Carpenter wrote:
> Hello Naoya Horiguchi,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 4c5bbbd24ae1: "mm: mbind: add hugepage migration code to
> mbind()" from Aug 16, 2013, leads to the following Smatch complaint:
>
> mm/mempolicy.c:1199 new_vma_page()
> error: we previously assumed 'vma' could be null (see line 1191)
>
> mm/mempolicy.c
> 1190
> 1191 while (vma) {
> ^^^
> Old check.
>
> 1192 address = page_address_in_vma(page, vma);
> 1193 if (address != -EFAULT)
> 1194 break;
> 1195 vma = vma->vm_next;
> 1196 }
> 1197
> 1198 if (PageHuge(page))
> 1199 return alloc_huge_page_noerr(vma, address, 1);
> ^^^
>
> New dereference inside the call to alloc_huge_page_noerr()
>
> 1200 /*
> 1201 * if !vma, alloc_page_vma() will use task or system default policy
I think that making alloc_huge_page_noerr() return NULL for !vma is one
possible solution. But current code looks strange to me in anther way,
and I don't think that considering vma==NULL case is meaningful.
When migrate_pages() is called from do_mbind(), pages in the pagelist are
collected via check_range(). And the collected pages certainly belong to
some vma. So it seems to me that something wrong should happen in !vma case.
So my suggestion is to add BUG_ON(!vma) after the while loop with comments.
Thanks,
Naoya Horiguchi
------------------------------------------------------------------------
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Mon, 19 Aug 2013 13:23:34 -0400
Subject: [PATCH][mmotm] mbind: add BUG_ON(!vma) in new_vma_page()
new_vma_page() is called only by page migration called from do_mbind(),
where pages to be migrated are queued into a pagelist by queue_pages_range().
queue_pages_range() confirms that a queued page belongs to some vma,
so !vma case is not supposed to be happen.
This patch adds BUG_ON() to catch this unexpected case.
Dependency:
"mempolicy: rename check_*range to queue_pages_*range"
in git//git.cmpxchg.org/linux-mmotm master
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
mm/mempolicy.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index dca5225..43a70dc 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1187,12 +1187,14 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
break;
vma = vma->vm_next;
}
+ /*
+ * queue_pages_range() confirms that @page belongs to some vma,
+ * so vma shouldn't be NULL.
+ */
+ BUG_ON(!vma);
if (PageHuge(page))
return alloc_huge_page_noerr(vma, address, 1);
- /*
- * if !vma, alloc_page_vma() will use task or system default policy
- */
return alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);
}
#else
--
1.8.3.1
--
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>
^ permalink raw reply related [flat|nested] 2+ messages in thread