Hi Naoya,
I noticed that your
mm-add-pte_present-check-on-existing-hugetlb_entry-callbacks.patch in
Andrew's -mm tree has been queued for a while and has a CC to stable on
it. Is that something that should get into 3.15? I know it doesn't
cleanly apply to Linus' current tree because of the patch before it, but
it seems to be a fairly independent fix.
This originally came up in this thread: https://lkml.org/lkml/2014/3/18/784
as a fix for some issues Sasha was hitting with the generic page walker
changes, but you found it was an existing issue. We now have a CVE
assigned for this:
http://seclists.org/oss-sec/2014/q2/399
So I'm wondering if you think this should fix the issue and if it should
go into 3.15. A backported version is below. I poked Linus about this
early today privately (my fault, apologies) and he had some
questions/comments on the code.
josh
>From ecc894926ef62080c2a4c4286eccce9d2f30f05a Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi <[email protected]>
Date: Fri, 6 Jun 2014 10:00:01 -0400
Subject: [PATCH] mm: add !pte_present() check on existing hugetlb_entry
callbacks
Page table walker doesn't check non-present hugetlb entry in common path,
so hugetlb_entry() callbacks must check it. The reason for this behavior
is that some callers want to handle it in its own way.
However, some callers don't check it now, which causes unpredictable
result, for example when we have a race between migrating hugepage and
reading /proc/pid/numa_maps. This patch fixes it by adding !pte_present
checks on buggy callbacks.
This bug exists for years and got visible by introducing hugepage migration.
ChangeLog v2:
- fix if condition (check !pte_present() instead of pte_present())
Reported-by: Sasha Levin <[email protected]>
Signed-off-by: Naoya Horiguchi <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: <[email protected]> [3.12+]
Signed-off-by: Andrew Morton <[email protected]>
[ Backported to 3.15. Signed-off-by: Josh Boyer <[email protected]> ]
---
fs/proc/task_mmu.c | 3 +++
mm/mempolicy.c | 6 +++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 442177b1119a..89620cdb57c9 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1354,6 +1354,9 @@ static int gather_hugetbl_stats(pte_t *pte, unsigned long hmask,
if (pte_none(*pte))
return 0;
+ if (!pte_present(*pte))
+ return 0;
+
page = pte_page(*pte);
if (!page)
return 0;
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 78e1472933ea..30cc47f8ffa0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -526,9 +526,13 @@ static void queue_pages_hugetlb_pmd_range(struct vm_area_struct *vma,
int nid;
struct page *page;
spinlock_t *ptl;
+ pte_t entry;
ptl = huge_pte_lock(hstate_vma(vma), vma->vm_mm, (pte_t *)pmd);
- page = pte_page(huge_ptep_get((pte_t *)pmd));
+ entry = huge_ptep_get((pte_t *)pmd);
+ if (!pte_present(entry))
+ goto unlock;
+ page = pte_page(entry);
nid = page_to_nid(page);
if (node_isset(nid, *nodes) == !!(flags & MPOL_MF_INVERT))
goto unlock;
--
1.9.3
On Fri, 6 Jun 2014 14:46:37 -0400 Naoya Horiguchi <[email protected]> wrote:
> On Fri, Jun 06, 2014 at 01:36:54PM -0400, Josh Boyer wrote:
> > Hi Naoya,
> >
> > I noticed that your
> > mm-add-pte_present-check-on-existing-hugetlb_entry-callbacks.patch in
> > Andrew's -mm tree has been queued for a while and has a CC to stable on
> > it. Is that something that should get into 3.15?
>
> I'm not sure. I guess Andrew will decide this in a few days.
I was planning on doing so but then Dave popped up with an alternative
which rather blew a hole in things.
On 06/06/2014 11:59 AM, Andrew Morton wrote:
> On Fri, 6 Jun 2014 14:46:37 -0400 Naoya Horiguchi <[email protected]> wrote:
>> On Fri, Jun 06, 2014 at 01:36:54PM -0400, Josh Boyer wrote:
>>> Hi Naoya,
>>>
>>> I noticed that your
>>> mm-add-pte_present-check-on-existing-hugetlb_entry-callbacks.patch in
>>> Andrew's -mm tree has been queued for a while and has a CC to stable on
>>> it. Is that something that should get into 3.15?
>>
>> I'm not sure. I guess Andrew will decide this in a few days.
>
> I was planning on doing so but then Dave popped up with an alternative
> which rather blew a hole in things.
This one patch is independent from what I was posting. I don't think my
patch would have had any effect on the behavior this one is fixing.
IOW, don't hold it up for my (or my patches) benefit.
On 06/06/2014 02:46 PM, Naoya Horiguchi wrote:
>> A backported version is below. I poked Linus about this
>> > early today privately (my fault, apologies) and he had some
>> > questions/comments on the code.
> I'm not sure the same problem really reproduces on mainline kernel without
> my page table walker patchset, because I could't reproduce it in my environment
> (Sasha, have you seen this problem on mainline kernel?)
I haven't been fuzzing the mainline kernel at any point, but this issue reproduced
with the page table walker patches reverted on -next, so I think it's safe to assume
it would reproduce on mainline.
Thanks,
Sasha
On Fri, 06 Jun 2014 13:11:00 -0700 Dave Hansen <[email protected]> wrote:
> On 06/06/2014 11:59 AM, Andrew Morton wrote:
> > On Fri, 6 Jun 2014 14:46:37 -0400 Naoya Horiguchi <[email protected]> wrote:
> >> On Fri, Jun 06, 2014 at 01:36:54PM -0400, Josh Boyer wrote:
> >>> Hi Naoya,
> >>>
> >>> I noticed that your
> >>> mm-add-pte_present-check-on-existing-hugetlb_entry-callbacks.patch in
> >>> Andrew's -mm tree has been queued for a while and has a CC to stable on
> >>> it. Is that something that should get into 3.15?
> >>
> >> I'm not sure. I guess Andrew will decide this in a few days.
> >
> > I was planning on doing so but then Dave popped up with an alternative
> > which rather blew a hole in things.
>
> This one patch is independent from what I was posting. I don't think my
> patch would have had any effect on the behavior this one is fixing.
>
> IOW, don't hold it up for my (or my patches) benefit.
I was referring to these, which remain in limbo:
mm-hugetlbfs-fix-rmapping-for-anonymous-hugepages-with-page_pgoff.patch
mm-hugetlbfs-fix-rmapping-for-anonymous-hugepages-with-page_pgoff-v2.patch
mm-hugetlbfs-fix-rmapping-for-anonymous-hugepages-with-page_pgoff-v3.patch
mm-hugetlbfs-fix-rmapping-for-anonymous-hugepages-with-page_pgoff-v3-fix.patch
pagewalk-update-page-table-walker-core.patch
pagewalk-update-page-table-walker-core-fix-end-address-calculation-in-walk_page_range.patch
pagewalk-update-page-table-walker-core-fix-end-address-calculation-in-walk_page_range-fix.patch
pagewalk-update-page-table-walker-core-fix.patch
pagewalk-add-walk_page_vma.patch
smaps-redefine-callback-functions-for-page-table-walker.patch
clear_refs-redefine-callback-functions-for-page-table-walker.patch
pagemap-redefine-callback-functions-for-page-table-walker.patch
pagemap-redefine-callback-functions-for-page-table-walker-fix.patch
numa_maps-redefine-callback-functions-for-page-table-walker.patch
memcg-redefine-callback-functions-for-page-table-walker.patch
arch-powerpc-mm-subpage-protc-use-walk_page_vma-instead-of-walk_page_range.patch
pagewalk-remove-argument-hmask-from-hugetlb_entry.patch
pagewalk-remove-argument-hmask-from-hugetlb_entry-fix.patch
pagewalk-remove-argument-hmask-from-hugetlb_entry-fix-fix.patch
mempolicy-apply-page-table-walker-on-queue_pages_range.patch
mm-add-pte_present-check-on-existing-hugetlb_entry-callbacks.patch
mm-pagewalkc-move-pte-null-check.patch
mm-prom-pid-clear_refs-avoid-split_huge_page.patch
Some energy in driving all this toward some sort of conclusion would be
appreciated.
On Fri, Jun 6, 2014 at 10:36 AM, Josh Boyer <[email protected]> wrote:
>
> I poked Linus about this
> early today privately (my fault, apologies) and he had some
> questions/comments on the code.
I changed it a bit (made the patch smaller by removing the now stale
"pte_none()" check) and added a comment about the apparent bogosity of
the generic page table walker to the commit message, but it's
basically applied to the current 3.15 git tree.
If some architecture really has a "pte_present()" function that
requires you to check "pte_none()" first, that architecture is
completely broken and would need to be fixed. I don't think that
exists, though.
Linus