could you clarify mm/mempolicy: fix !vma in new_vma_page()

[Michal Hocko]

Hi Wanpeng Li,
I have just noticed 11c731e81bb0 (mm/mempolicy: fix !vma in
new_vma_page()) and I am not sure I understand it. Your changelog claims
"
page_address_in_vma() may still return -EFAULT because of many other
conditions in it. As a result the while loop in new_vma_page() may end
with vma=NULL.
"

And the patch handles hugetlb case only. I was wondering what are those
"other conditions" that failed in the BUG_ON mentioned in the changelog?
Could you be more specific please?

Thanks!
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bob Liu]

Hi Michal,

On Mon, Jan 6, 2014 at 7:24 PM, Michal Hocko <mho...@suse.cz> wrote:
> Hi Wanpeng Li,
> I have just noticed 11c731e81bb0 (mm/mempolicy: fix !vma in
> new_vma_page()) and I am not sure I understand it. Your changelog claims
> "
> page_address_in_vma() may still return -EFAULT because of many other
> conditions in it. As a result the while loop in new_vma_page() may end
> with vma=NULL.
> "
>
> And the patch handles hugetlb case only. I was wondering what are those
> "other conditions" that failed in the BUG_ON mentioned in the changelog?
> Could you be more specific please?
>

Sorry for the confusion caused.
The code of new_vma_page() used to like this:
1193 while (vma) {
1194 address = page_address_in_vma(page, vma);
1195 if (address != -EFAULT)
1196 break;
1197 vma = vma->vm_next;
1198 }
1199 /*
1200 * queue_pages_range() confirms that @page belongs to some vma,
1201 * so vma shouldn't be NULL.
1202 */
1203 BUG_ON(!vma);
1204
1205 if (PageHuge(page))
1206 return alloc_huge_page_noerr(vma, address, 1);
1207 return alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);

The BUG_ON() was triggered and my idea was that even
queue_pages_range() confirms @page belongs to some vma,
page_address_in_vma() may still return -EFAULT because of below checks
in page_address_in_vma().

544 if (PageAnon(page)) {
545 struct anon_vma *page__anon_vma = page_anon_vma(page);
546 /*
547 * Note: swapoff's unuse_vma() is more efficient with this
548 * check, and needs it to match anon_vma when KSM
is active.
549 */
550 if (!vma->anon_vma || !page__anon_vma ||
551 vma->anon_vma->root != page__anon_vma->root)
552 return -EFAULT;
553 } else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
554 if (!vma->vm_file ||
555 vma->vm_file->f_mapping != page->mapping)
556 return -EFAULT;
557 } else
558 return -EFAULT;

That's the "other conditions" and the reason why we can't use
BUG_ON(!vma) in new_vma_page().

--
Regards,
--Bob

[Michal Hocko]

On Mon 06-01-14 20:45:54, Bob Liu wrote:
[...]

> 544 if (PageAnon(page)) {
> 545 struct anon_vma *page__anon_vma = page_anon_vma(page);
> 546 /*
> 547 * Note: swapoff's unuse_vma() is more efficient with this
> 548 * check, and needs it to match anon_vma when KSM is active.
> 549 */
> 550 if (!vma->anon_vma || !page__anon_vma ||
> 551 vma->anon_vma->root != page__anon_vma->root)
> 552 return -EFAULT;
> 553 } else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
> 554 if (!vma->vm_file ||
> 555 vma->vm_file->f_mapping != page->mapping)
> 556 return -EFAULT;
> 557 } else
> 558 return -EFAULT;
>
> That's the "other conditions" and the reason why we can't use
> BUG_ON(!vma) in new_vma_page().

Sorry, I wasn't clear with my question. I was interested in which of
these triggered and why only for hugetlb pages?

--
Michal Hocko
SUSE Labs

[Bob Liu]

On Mon, Jan 6, 2014 at 10:18 PM, Michal Hocko <mho...@suse.cz> wrote:
> On Mon 06-01-14 20:45:54, Bob Liu wrote:
> [...]
>> 544 if (PageAnon(page)) {
>> 545 struct anon_vma *page__anon_vma = page_anon_vma(page);
>> 546 /*
>> 547 * Note: swapoff's unuse_vma() is more efficient with this
>> 548 * check, and needs it to match anon_vma when KSM is active.
>> 549 */
>> 550 if (!vma->anon_vma || !page__anon_vma ||
>> 551 vma->anon_vma->root != page__anon_vma->root)
>> 552 return -EFAULT;
>> 553 } else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
>> 554 if (!vma->vm_file ||
>> 555 vma->vm_file->f_mapping != page->mapping)
>> 556 return -EFAULT;
>> 557 } else
>> 558 return -EFAULT;
>>
>> That's the "other conditions" and the reason why we can't use
>> BUG_ON(!vma) in new_vma_page().
>
> Sorry, I wasn't clear with my question. I was interested in which of
> these triggered and why only for hugetlb pages?
>

Sorry I didn't analyse the root cause. They are several checks in
page_address_in_vma() so I think it might be not difficult to hit one
of them. For example, if the page was mapped to vma by nonlinear
mapping?
Anyway, some debug code is needed to verify what really happened here.

alloc_page_vma() can handle the vma=NULL case while
alloc_huge_page_noerr() can't, so we return NULL instead of call down
to alloc_huge_page().

--
Regards,
--Bob

[Michal Hocko]

On Tue 07-01-14 12:34:34, Wanpeng Li wrote:
> Cced Sasha,

> On Tue, Jan 07, 2014 at 12:26:13PM +0800, Wanpeng Li wrote:
> >Hi Michal,

> >On Mon, Jan 06, 2014 at 03:18:27PM +0100, Michal Hocko wrote:
> >>On Mon 06-01-14 20:45:54, Bob Liu wrote:
> >>[...]
> >>> 544 if (PageAnon(page)) {
> >>> 545 struct anon_vma *page__anon_vma = page_anon_vma(page);
> >>> 546 /*
> >>> 547 * Note: swapoff's unuse_vma() is more efficient with this
> >>> 548 * check, and needs it to match anon_vma when KSM is active.
> >>> 549 */
> >>> 550 if (!vma->anon_vma || !page__anon_vma ||
> >>> 551 vma->anon_vma->root != page__anon_vma->root)
> >>> 552 return -EFAULT;
> >>> 553 } else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
> >>> 554 if (!vma->vm_file ||
> >>> 555 vma->vm_file->f_mapping != page->mapping)
> >>> 556 return -EFAULT;
> >>> 557 } else
> >>> 558 return -EFAULT;
> >>>
> >>> That's the "other conditions" and the reason why we can't use
> >>> BUG_ON(!vma) in new_vma_page().
> >>
> >>Sorry, I wasn't clear with my question. I was interested in which of
> >>these triggered and why only for hugetlb pages?
> >

> >Not just for hugetlb pages, sorry for do two things in one patch. The change
> >for hugetlb pages is to fix the potential dereference NULL pointer reported
> >by Dan. http://marc.info/?l=linux-mm&m=137689530323257&w=2
> >
> >If we should ask Sasha to add more debug information to dump which condition
> >is failed in page_address_in_vma() for you?

I am always more calm when the removed BUG_ON is properly understood and
justified.

[Michal Hocko]

I would be really curious when anon_vma or f_mapping would be out of
sync, that's why I've asked in the first place.

> For example, if the page was mapped to vma by nonlinear
> mapping?

Hmm, ok !private shmem/hugetlbfs might be remapped as non-linear. For
some reason I thought that migration for non-linear mappings is not
allowed. This is not the case and it would explain why the BUG_ON
triggered.

> Anyway, some debug code is needed to verify what really happened here.

That would be prefferable before the patch had been submitted and
merged...

> alloc_page_vma() can handle the vma=NULL case while
> alloc_huge_page_noerr() can't, so we return NULL instead of call down
> to alloc_huge_page().

OK, I see.

Thanks!

--
Michal Hocko
SUSE Labs

[Michal Hocko]

OK, it didn't let go away from my head so I had to check. hugetlbfs
cannot be remmaped as non-linear because it is missing its vm_ops is
missing remap_pages implementation. So this case is impossible for these
pages. So at least the PageHuge part of the patch is bogus AFAICS.

We still have shmem and even then I am curious whether we are doing the
right thing. The loop is inteded to handle range spanning multiple VMAs
(as per 3ad33b2436b54 (Migration: find correct vma in new_vma_page()))
and it doesn't seem to be VM_NONLINEAR aware. It will always fail for
shared shmem and so we always fallback to task/system default mempolicy.
Whether somebody uses mempolicy on VM_NONLINEAR mappings is hard to
tell. I am not familiar with this feature much.

That being said. The BUG_ON(!vma) was bogus for VM_NONLINEAR cases.
The changed code could keep it for hugetlbfs path because we shouldn't
see NULL vma there AFAICS.

What is the right(tm) thing to do for VM_NONLINEAR is hard to tell and I
would leave it to those who are more familiar with the usage.

[Bob Liu]

Sounds reasonable, but as your said we'd better find out the root
cause before making any changes.
Do you think below debug info is enough? If yes, then we can ask Sasha
help us having a test.

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 12733f5..86c5cc0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1189,11 +1189,21 @@ static struct page *new_vma_page(struct page
*page, unsigned long private, int *
{
struct vm_area_struct *vma = (struct vm_area_struct *)private;
unsigned long uninitialized_var(address);
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 12733f5..86c5cc0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1189,11 +1189,21 @@ static struct page *new_vma_page(struct page
*page, unsigned long private, int *
{
struct vm_area_struct *vma = (struct vm_area_struct *)private;
unsigned long uninitialized_var(address);
+ unsigned long uninitialized_var(address2);

while (vma) {
address = page_address_in_vma(page, vma);
if (address != -EFAULT)
break;
+#if 1
+ address2 = vma_address(page, vma);
+ if (address2 >= vma->vm_start && address2 < vma->vm_end) {
+ printk("other condition happened\n");
+ if (vma->vm_flags & VM_NONLINEAR)
+ printk("non linear map\n");
+ dump_page(page);
+ }
+#endif
vma = vma->vm_next;
}
/*
diff --git a/mm/rmap.c b/mm/rmap.c
index d792e71..4d35d5c 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -529,7 +529,7 @@ vma_address(struct page *page, struct vm_area_struct *vma)
unsigned long address = __vma_address(page, vma);

/* page should be within @vma mapping range */
- VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);
+ //VM_BUG_ON(address < vma->vm_start || address >= vma->vm_end);

return address;
}

--
Regards,
--Bob

[Michal Hocko]

If I was debugging this I would simply add printk into page_address_in_vma
error paths.

Anyway, I think that at least hugetlbfs part should be reverted because
it might paper over real bugs. Although the migration would fail for
such hugetlb page we should catch that a weird page was tried to be
migrated. What about the patch below?
---
From 2d61421f26a3b63b4670d71b7adc67e2191b6157 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mho...@suse.cz>
Date: Wed, 8 Jan 2014 10:57:41 +0100
Subject: [PATCH] mm: new_vma_page cannot see NULL vma for hugetlb pages

11c731e81bb0 (mm/mempolicy: fix !vma in new_vma_page()) has removed
BUG_ON(!vma) from new_vma_page which is partially correct because
page_address_in_vma will return EFAULT for non-linear mappings and at
least shared shmem might be mapped this way.

The patch also tried to prevent NULL ptr for hugetlb pages which is not
correct AFAICS because hugetlb pages cannot be mapped as VM_NONLINEAR
and other conditions in page_address_in_vma seem to be legit and catch
real bugs.

This patch restores BUG_ON for PageHuge to catch potential issues when
the to-be-migrated page is not setup properly.

Signed-off-by: Michal Hocko <mho...@suse.cz>
---
mm/mempolicy.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 9e8d2d86978a..f3f51464a23b 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1199,10 +1199,8 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
}

if (PageHuge(page)) {
- if (vma)
- return alloc_huge_page_noerr(vma, address, 1);
- else
- return NULL;
+ BUG_ON(vma)
+ return alloc_huge_page_noerr(vma, address, 1);
}
/*
* if !vma, alloc_page_vma() will use task or system default policy
--
1.8.5.2

--
Michal Hocko
SUSE Labs

[Bob Liu]

On Wed, Jan 8, 2014 at 6:08 PM, Michal Hocko <mho...@suse.cz> wrote:

>
> If I was debugging this I would simply add printk into page_address_in_vma
> error paths.
>
> Anyway, I think that at least hugetlbfs part should be reverted because
> it might paper over real bugs. Although the migration would fail for
> such hugetlb page we should catch that a weird page was tried to be
> migrated. What about the patch below?

Looks good to me. But we need to confirm whether our assumption is right.
Sasha, could you please have a test with Michal's patch?

Thanks,
-Bob

[Michal Hocko]

On Wed 08-01-14 20:09:30, Bob Liu wrote:
> On Wed, Jan 8, 2014 at 6:08 PM, Michal Hocko <mho...@suse.cz> wrote:
>
> >
> > If I was debugging this I would simply add printk into page_address_in_vma
> > error paths.
> >
> > Anyway, I think that at least hugetlbfs part should be reverted because
> > it might paper over real bugs. Although the migration would fail for
> > such hugetlb page we should catch that a weird page was tried to be
> > migrated. What about the patch below?
>
> Looks good to me. But we need to confirm whether our assumption is right.

Which assumption you have in mind? non-linear mapping or failing on
anon_vma or f_mapping checks?

> Sasha, could you please have a test with Michal's patch?

I obviously doesn't have anything against testing but we should really
focus on the original issue. This patch simply restores hugetlb code
path.

[Bob Liu]

On Wed, Jan 8, 2014 at 8:42 PM, Michal Hocko <mho...@suse.cz> wrote:
> On Wed 08-01-14 20:09:30, Bob Liu wrote:
>> On Wed, Jan 8, 2014 at 6:08 PM, Michal Hocko <mho...@suse.cz> wrote:
>>
>> >
>> > If I was debugging this I would simply add printk into page_address_in_vma
>> > error paths.
>> >
>> > Anyway, I think that at least hugetlbfs part should be reverted because
>> > it might paper over real bugs. Although the migration would fail for
>> > such hugetlb page we should catch that a weird page was tried to be
>> > migrated. What about the patch below?
>>
>> Looks good to me. But we need to confirm whether our assumption is right.
>
> Which assumption you have in mind? non-linear mapping or failing on
> anon_vma or f_mapping checks?
>

The assumption that the original BUG_ON(!vma) was triggered by
non-linear mapping.

>> Sasha, could you please have a test with Michal's patch?
>
> I obviously doesn't have anything against testing but we should really
> focus on the original issue. This patch simply restores hugetlb code

Oh, I see your point.
Yes, I agree that your patch should be merged and if the BUG_ON() is
triggered(which is unlikely) again.
We can open another thread and analysis the root cause.

>> > ---
>> > From 2d61421f26a3b63b4670d71b7adc67e2191b6157 Mon Sep 17 00:00:00 2001
>> > From: Michal Hocko <mho...@suse.cz>
>> > Date: Wed, 8 Jan 2014 10:57:41 +0100
>> > Subject: [PATCH] mm: new_vma_page cannot see NULL vma for hugetlb pages
>> >
>> > 11c731e81bb0 (mm/mempolicy: fix !vma in new_vma_page()) has removed
>> > BUG_ON(!vma) from new_vma_page which is partially correct because
>> > page_address_in_vma will return EFAULT for non-linear mappings and at
>> > least shared shmem might be mapped this way.
>> >
>> > The patch also tried to prevent NULL ptr for hugetlb pages which is not
>> > correct AFAICS because hugetlb pages cannot be mapped as VM_NONLINEAR
>> > and other conditions in page_address_in_vma seem to be legit and catch
>> > real bugs.
>> >
>> > This patch restores BUG_ON for PageHuge to catch potential issues when
>> > the to-be-migrated page is not setup properly.
>> >
>> > Signed-off-by: Michal Hocko <mho...@suse.cz>

Reviewed-by: Bob Liu <bob...@oracle.com>

>> > ---
>> > mm/mempolicy.c | 6 ++----
>> > 1 file changed, 2 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> > index 9e8d2d86978a..f3f51464a23b 100644
>> > --- a/mm/mempolicy.c
>> > +++ b/mm/mempolicy.c
>> > @@ -1199,10 +1199,8 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
>> > }
>> >
>> > if (PageHuge(page)) {
>> > - if (vma)
>> > - return alloc_huge_page_noerr(vma, address, 1);
>> > - else
>> > - return NULL;
>> > + BUG_ON(vma)
>> > + return alloc_huge_page_noerr(vma, address, 1);
>> > }
>> > /*
>> > * if !vma, alloc_page_vma() will use task or system default policy
>> > --
>> > 1.8.5.2
>> >

--

Regards,
--Bob

[Michal Hocko]

Hi Andrew,
the whole thread started here: http://lkml.org/lkml/2014/1/6/217
I guess it makes sense to revert part of the already merged commit with
the following patch. If the BUG_ON triggers again then we should rather
find out why page_address_in_vma fails on anon_vma or f_mapping checks
and not simply paper over it.
---
From 805035f35e8865f6233f88c78e7063512042afea Mon Sep 17 00:00:00 2001

From: Michal Hocko <mho...@suse.cz>
Date: Wed, 8 Jan 2014 10:57:41 +0100
Subject: [PATCH] mm: new_vma_page cannot see NULL vma for hugetlb pages

11c731e81bb0 (mm/mempolicy: fix !vma in new_vma_page()) has removed
BUG_ON(!vma) from new_vma_page which is partially correct because
page_address_in_vma will return EFAULT for non-linear mappings and at
least shared shmem might be mapped this way.

The patch also tried to prevent NULL ptr for hugetlb pages which is not
correct AFAICS because hugetlb pages cannot be mapped as VM_NONLINEAR
and other conditions in page_address_in_vma seem to be legit and catch
real bugs.

This patch restores BUG_ON for PageHuge to catch potential issues when
the to-be-migrated page is not setup properly.

Signed-off-by: Michal Hocko <mho...@suse.cz>
Reviewed-by: Bob Liu <bob...@oracle.com>
---
mm/mempolicy.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c

index 9e8d2d86978a..1a368cd925ed 100644

--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1199,10 +1199,8 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
}

if (PageHuge(page)) {
- if (vma)
- return alloc_huge_page_noerr(vma, address, 1);
- else
- return NULL;

+ BUG_ON(!vma);

+ return alloc_huge_page_noerr(vma, address, 1);
}
/*
* if !vma, alloc_page_vma() will use task or system default policy
--
1.8.5.2

--

Michal Hocko
SUSE Labs

[Michal Hocko]

On Wed 08-01-14 21:10:29, Bob Liu wrote:
[...]

> >> > From 2d61421f26a3b63b4670d71b7adc67e2191b6157 Mon Sep 17 00:00:00 2001
> >> > From: Michal Hocko <mho...@suse.cz>
> >> > Date: Wed, 8 Jan 2014 10:57:41 +0100
> >> > Subject: [PATCH] mm: new_vma_page cannot see NULL vma for hugetlb pages
> >> >
> >> > 11c731e81bb0 (mm/mempolicy: fix !vma in new_vma_page()) has removed
> >> > BUG_ON(!vma) from new_vma_page which is partially correct because
> >> > page_address_in_vma will return EFAULT for non-linear mappings and at
> >> > least shared shmem might be mapped this way.
> >> >
> >> > The patch also tried to prevent NULL ptr for hugetlb pages which is not
> >> > correct AFAICS because hugetlb pages cannot be mapped as VM_NONLINEAR
> >> > and other conditions in page_address_in_vma seem to be legit and catch
> >> > real bugs.
> >> >
> >> > This patch restores BUG_ON for PageHuge to catch potential issues when
> >> > the to-be-migrated page is not setup properly.
> >> >
> >> > Signed-off-by: Michal Hocko <mho...@suse.cz>
>
> Reviewed-by: Bob Liu <bob...@oracle.com>

Thanks!

> >> > ---
> >> > mm/mempolicy.c | 6 ++----
> >> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> >> > index 9e8d2d86978a..f3f51464a23b 100644
> >> > --- a/mm/mempolicy.c
> >> > +++ b/mm/mempolicy.c
> >> > @@ -1199,10 +1199,8 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
> >> > }
> >> >
> >> > if (PageHuge(page)) {
> >> > - if (vma)
> >> > - return alloc_huge_page_noerr(vma, address, 1);
> >> > - else
> >> > - return NULL;
> >> > + BUG_ON(vma)

That was meant to say BUG_ON(!vma) of course ;) but I guess your
reviewed-by still applies so I will post it to Andrew.

> >> > + return alloc_huge_page_noerr(vma, address, 1);
> >> > }
> >> > /*
> >> > * if !vma, alloc_page_vma() will use task or system default policy
> >> > --
> >> > 1.8.5.2
> >> >
>
> --
> Regards,
> --Bob

--

Michal Hocko
SUSE Labs