[PATCH] hugetlb: detect race if fail to COW

[PATCH] hugetlb: detect race if fail to COW

[Hillf Danton]

In the error path that we fail to allocate new huge page, before try again, we
have to check race since page_table_lock is re-acquired.

If racing, our job is done.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/mm/hugetlb.c	Fri Nov 18 21:38:30 2011
+++ b/mm/hugetlb.c	Fri Nov 18 21:48:15 2011
@@ -2407,7 +2407,14 @@ retry_avoidcopy:
 				BUG_ON(page_count(old_page) != 1);
 				BUG_ON(huge_pte_none(pte));
 				spin_lock(&mm->page_table_lock);
-				goto retry_avoidcopy;
+				ptep = huge_pte_offset(mm, address & huge_page_mask(h));
+				if (likely(pte_same(huge_ptep_get(ptep), pte)))
+					goto retry_avoidcopy;
+				/*
+				 * race occurs while re-acquiring page_table_lock, and
+				 * our job is done.
+				 */
+				return 0;
 			}
 			WARN_ON_ONCE(1);
 		}

Re: [PATCH] hugetlb: detect race if fail to COW

[John Kacur]

On Fri, Nov 18, 2011 at 3:04 PM, Hillf Danton <dhillf@gmail.com> wrote:
> In the error path that we fail to allocate new huge page, before try again, we
> have to check race since page_table_lock is re-acquired.
>
> If racing, our job is done.
>
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>
> --- a/mm/hugetlb.c      Fri Nov 18 21:38:30 2011
> +++ b/mm/hugetlb.c      Fri Nov 18 21:48:15 2011
> @@ -2407,7 +2407,14 @@ retry_avoidcopy:
>                                BUG_ON(page_count(old_page) != 1);
>                                BUG_ON(huge_pte_none(pte));
>                                spin_lock(&mm->page_table_lock);
> -                               goto retry_avoidcopy;
> +                               ptep = huge_pte_offset(mm, address & huge_page_mask(h));
> +                               if (likely(pte_same(huge_ptep_get(ptep), pte)))
> +                                       goto retry_avoidcopy;
> +                               /*
> +                                * race occurs while re-acquiring page_table_lock, and
> +                                * our job is done.
> +                                */
> +                               return 0;
>                        }
>                        WARN_ON_ONCE(1);
>                }


I'm not sure about the veracity of the race condition, but you better
do spin_unlock before you return.

Re: [PATCH] hugetlb: detect race if fail to COW

[John Kacur]

On Fri, Nov 18, 2011 at 3:16 PM, John Kacur <jkacur@redhat.com> wrote:
> On Fri, Nov 18, 2011 at 3:04 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> In the error path that we fail to allocate new huge page, before try again, we
>> have to check race since page_table_lock is re-acquired.
>>
>> If racing, our job is done.
>>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- a/mm/hugetlb.c      Fri Nov 18 21:38:30 2011
>> +++ b/mm/hugetlb.c      Fri Nov 18 21:48:15 2011
>> @@ -2407,7 +2407,14 @@ retry_avoidcopy:
>>                                BUG_ON(page_count(old_page) != 1);
>>                                BUG_ON(huge_pte_none(pte));
>>                                spin_lock(&mm->page_table_lock);
>> -                               goto retry_avoidcopy;
>> +                               ptep = huge_pte_offset(mm, address & huge_page_mask(h));
>> +                               if (likely(pte_same(huge_ptep_get(ptep), pte)))
>> +                                       goto retry_avoidcopy;
>> +                               /*
>> +                                * race occurs while re-acquiring page_table_lock, and
>> +                                * our job is done.
>> +                                */
>> +                               return 0;
>>                        }
>>                        WARN_ON_ONCE(1);
>>                }
>
>
> I'm not sure about the veracity of the race condition, but you better
> do spin_unlock before you return.
>

Ugh, sorry for the noise, I see that's not how it works here.

Re: [PATCH] hugetlb: detect race if fail to COW

[Hillf Danton]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2069 bytes --]

On Fri, Nov 18, 2011 at 10:21 PM, John Kacur <jkacur@redhat.com> wrote:
> On Fri, Nov 18, 2011 at 3:16 PM, John Kacur <jkacur@redhat.com> wrote:
>> On Fri, Nov 18, 2011 at 3:04 PM, Hillf Danton <dhillf@gmail.com> wrote:
>>> In the error path that we fail to allocate new huge page, before try again, we
>>> have to check race since page_table_lock is re-acquired.
>>>
>>> If racing, our job is done.
>>>
>>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>>> ---
>>>
>>> --- a/mm/hugetlb.c      Fri Nov 18 21:38:30 2011
>>> +++ b/mm/hugetlb.c      Fri Nov 18 21:48:15 2011
>>> @@ -2407,7 +2407,14 @@ retry_avoidcopy:
>>>                                BUG_ON(page_count(old_page) != 1);
>>>                                BUG_ON(huge_pte_none(pte));
>>>                                spin_lock(&mm->page_table_lock);
>>> -                               goto retry_avoidcopy;
>>> +                               ptep = huge_pte_offset(mm, address & huge_page_mask(h));
>>> +                               if (likely(pte_same(huge_ptep_get(ptep), pte)))
>>> +                                       goto retry_avoidcopy;
>>> +                               /*
>>> +                                * race occurs while re-acquiring page_table_lock, and
>>> +                                * our job is done.
>>> +                                */
>>> +                               return 0;
>>>                        }
>>>                        WARN_ON_ONCE(1);
>>>                }
>>
>>
>> I'm not sure about the veracity of the race condition, but you better
>> do spin_unlock before you return.
>>
>
> Ugh, sorry for the noise, I see that's not how it works here.

Welcome:)
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] hugetlb: detect race if fail to COW

[Michal Hocko]

On Fri 18-11-11 22:04:37, Hillf Danton wrote:
> In the error path that we fail to allocate new huge page, before try again, we
> have to check race since page_table_lock is re-acquired.

I do not think we can race here because we are serialized by
hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
fall into avoidcopy and shortcut despite the fact that other thread has
already did the job.

The mutex usage is not obvious in hugetlb_cow so maybe we want to be
explicit about it (either a comment or do the recheck).

> 
> If racing, our job is done.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/mm/hugetlb.c	Fri Nov 18 21:38:30 2011
> +++ b/mm/hugetlb.c	Fri Nov 18 21:48:15 2011
> @@ -2407,7 +2407,14 @@ retry_avoidcopy:
>  				BUG_ON(page_count(old_page) != 1);
>  				BUG_ON(huge_pte_none(pte));
>  				spin_lock(&mm->page_table_lock);
> -				goto retry_avoidcopy;
> +				ptep = huge_pte_offset(mm, address & huge_page_mask(h));
> +				if (likely(pte_same(huge_ptep_get(ptep), pte)))
> +					goto retry_avoidcopy;
> +				/*
> +				 * race occurs while re-acquiring page_table_lock, and
> +				 * our job is done.
> +				 */
> +				return 0;
>  			}
>  			WARN_ON_ONCE(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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] hugetlb: detect race if fail to COW

[Hillf Danton]

On Fri, Nov 18, 2011 at 11:07 PM, Michal Hocko <mhocko@suse.cz> wrote:
> On Fri 18-11-11 22:04:37, Hillf Danton wrote:
>> In the error path that we fail to allocate new huge page, before try again, we
>> have to check race since page_table_lock is re-acquired.
>
> I do not think we can race here because we are serialized by
> hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
> fall into avoidcopy and shortcut despite the fact that other thread has
> already did the job.
>
> The mutex usage is not obvious in hugetlb_cow so maybe we want to be
> explicit about it (either a comment or do the recheck).
>

Then the following check is unnecessary, no?

Thanks,
Hillf

	/*
	 * Retake the page_table_lock to check for racing updates
	 * before the page tables are altered
	 */
	spin_lock(&mm->page_table_lock);
	ptep = huge_pte_offset(mm, address & huge_page_mask(h));
	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
		/* Break COW */

Re: [PATCH] hugetlb: detect race if fail to COW

[Michal Hocko]

On Fri 18-11-11 23:23:12, Hillf Danton wrote:
> On Fri, Nov 18, 2011 at 11:07 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Fri 18-11-11 22:04:37, Hillf Danton wrote:
> >> In the error path that we fail to allocate new huge page, before try again, we
> >> have to check race since page_table_lock is re-acquired.
> >
> > I do not think we can race here because we are serialized by
> > hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
> > fall into avoidcopy and shortcut despite the fact that other thread has
> > already did the job.
> >
> > The mutex usage is not obvious in hugetlb_cow so maybe we want to be
> > explicit about it (either a comment or do the recheck).
> >
> 
> Then the following check is unnecessary, no?

Hmm, thinking about it some more, I guess we have to recheck because we
can still race with page migration. So we need you patch.

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> 
> Thanks,
> Hillf
> 
> 	/*
> 	 * Retake the page_table_lock to check for racing updates
> 	 * before the page tables are altered
> 	 */
> 	spin_lock(&mm->page_table_lock);
> 	ptep = huge_pte_offset(mm, address & huge_page_mask(h));
> 	if (likely(pte_same(huge_ptep_get(ptep), pte))) {
> 		/* Break COW */
> 
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] hugetlb: detect race if fail to COW

[Andrew Morton]

On Fri, 18 Nov 2011 17:11:28 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Fri 18-11-11 23:23:12, Hillf Danton wrote:
> > On Fri, Nov 18, 2011 at 11:07 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > > On Fri 18-11-11 22:04:37, Hillf Danton wrote:
> > >> In the error path that we fail to allocate new huge page, before try again, we
> > >> have to check race since page_table_lock is re-acquired.
> > >
> > > I do not think we can race here because we are serialized by
> > > hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
> > > fall into avoidcopy and shortcut despite the fact that other thread has
> > > already did the job.
> > >
> > > The mutex usage is not obvious in hugetlb_cow so maybe we want to be
> > > explicit about it (either a comment or do the recheck).
> > >
> > 
> > Then the following check is unnecessary, no?
> 
> Hmm, thinking about it some more, I guess we have to recheck because we
> can still race with page migration. So we need you patch.
> 
> Reviewed-by: Michal Hocko <mhocko@suse.cz>

So we need a new changelog.  How does this look?


From: Hillf Danton <dhillf@gmail.com>
Subject: hugetlb: detect race upon page allocation failure during COW

In the error path where we failed to allocate a new huge page, we should
check whether a racing thread has added this page for us while this thread
waited for the page_table_lock.

We are serialized by hugetlb_instantiation_mutex on the pagefault patch
but this race can occur when another thread is performing page migration.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
Reviewed-by: Michal Hocko <mhocko@suse.cz>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Johannes Weiner <jweiner@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/hugetlb.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff -puN mm/hugetlb.c~hugetlb-detect-race-upon-page-allocation-failure-during-cow mm/hugetlb.c
--- a/mm/hugetlb.c~hugetlb-detect-race-upon-page-allocation-failure-during-cow
+++ a/mm/hugetlb.c
@@ -2407,7 +2407,14 @@ retry_avoidcopy:
 				BUG_ON(page_count(old_page) != 1);
 				BUG_ON(huge_pte_none(pte));
 				spin_lock(&mm->page_table_lock);
-				goto retry_avoidcopy;
+				ptep = huge_pte_offset(mm, address & huge_page_mask(h));
+				if (likely(pte_same(huge_ptep_get(ptep), pte)))
+					goto retry_avoidcopy;
+				/*
+				 * race occurs while re-acquiring page_table_lock, and
+				 * our job is done.
+				 */
+				return 0;
 			}
 			WARN_ON_ONCE(1);
 		}
_

Re: [PATCH] hugetlb: detect race if fail to COW

[Hillf Danton]

On Sat, Nov 19, 2011 at 3:39 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 18 Nov 2011 17:11:28 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
>
>> On Fri 18-11-11 23:23:12, Hillf Danton wrote:
>> > On Fri, Nov 18, 2011 at 11:07 PM, Michal Hocko <mhocko@suse.cz> wrote:
>> > > On Fri 18-11-11 22:04:37, Hillf Danton wrote:
>> > >> In the error path that we fail to allocate new huge page, before try again, we
>> > >> have to check race since page_table_lock is re-acquired.
>> > >
>> > > I do not think we can race here because we are serialized by
>> > > hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
>> > > fall into avoidcopy and shortcut despite the fact that other thread has
>> > > already did the job.
>> > >
>> > > The mutex usage is not obvious in hugetlb_cow so maybe we want to be
>> > > explicit about it (either a comment or do the recheck).
>> > >
>> >
>> > Then the following check is unnecessary, no?
>>
>> Hmm, thinking about it some more, I guess we have to recheck because we
>> can still race with page migration. So we need you patch.
>>
>> Reviewed-by: Michal Hocko <mhocko@suse.cz>
>
> So we need a new changelog.  How does this look?
>
Thanks Andrew and Michal:)

Best regards
Hillf

Re: [PATCH] hugetlb: detect race if fail to COW

[Michal Hocko]

On Fri 18-11-11 17:11:28, Michal Hocko wrote:
> On Fri 18-11-11 23:23:12, Hillf Danton wrote:
> > On Fri, Nov 18, 2011 at 11:07 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > > On Fri 18-11-11 22:04:37, Hillf Danton wrote:
> > >> In the error path that we fail to allocate new huge page, before try again, we
> > >> have to check race since page_table_lock is re-acquired.
> > >
> > > I do not think we can race here because we are serialized by
> > > hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
> > > fall into avoidcopy and shortcut despite the fact that other thread has
> > > already did the job.
> > >
> > > The mutex usage is not obvious in hugetlb_cow so maybe we want to be
> > > explicit about it (either a comment or do the recheck).
> > >
> > 
> > Then the following check is unnecessary, no?
> 
> Hmm, thinking about it some more, I guess we have to recheck because we
> can still race with page migration. So we need you patch.

OK, so looked at it again and we cannot race with page migration because
the page is locked (by unmap_and_move_*page) migration and we have the
old page locked here as well (hugetlb_fault).

Or am I missing something?

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] hugetlb: detect race if fail to COW

[Michal Hocko]

On Mon 21-11-11 13:23:03, Michal Hocko wrote:
> On Fri 18-11-11 17:11:28, Michal Hocko wrote:
> > On Fri 18-11-11 23:23:12, Hillf Danton wrote:
> > > On Fri, Nov 18, 2011 at 11:07 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > > > On Fri 18-11-11 22:04:37, Hillf Danton wrote:
> > > >> In the error path that we fail to allocate new huge page, before try again, we
> > > >> have to check race since page_table_lock is re-acquired.
> > > >
> > > > I do not think we can race here because we are serialized by
> > > > hugetlb_instantiation_mutex AFAIU. Without this lock, however, we could
> > > > fall into avoidcopy and shortcut despite the fact that other thread has
> > > > already did the job.
> > > >
> > > > The mutex usage is not obvious in hugetlb_cow so maybe we want to be
> > > > explicit about it (either a comment or do the recheck).
> > > >
> > > 
> > > Then the following check is unnecessary, no?
> > 
> > Hmm, thinking about it some more, I guess we have to recheck because we
> > can still race with page migration. So we need you patch.
> 
> OK, so looked at it again and we cannot race with page migration because
> the page is locked (by unmap_and_move_*page) migration and we have the
> old page locked here as well (hugetlb_fault).
> 
> Or am I missing something?

And the updated patch:
--- 
>From 1388a321b2a04643c777b613512e709a3519d3cc Mon Sep 17 00:00:00 2001
From: Hillf Danton <dhillf@gmail.com>
Date: Fri, 18 Nov 2011 22:04:37 +0800
Subject: [PATCH] hugetlb: detect race if fail to COW

Currently we are not rechecking pte_same in hugetlb_cow after we take
ptl lock again in the page allocation failure code path and simply retry
again. This is not an issue at the moment because hugetlb fault path
is protected by hugetlb_instantiation_mutex so we cannot race.

The original page is locked and so we cannot race even with the page
migration.

Let's add the pte_same check anyway as we want to be consistent with the
other check later in this function and be safe if we ever remove the
mutex.

[Michal Hocko <mhocko@suse.cz>: Reworded the changelog]
Signed-off-by: Hillf Danton <dhillf@gmail.com>
Reviewed-by: Michal Hocko <mhocko@suse.cz>
---
 mm/hugetlb.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bb28a5f..f76c7ea 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2407,7 +2407,14 @@ retry_avoidcopy:
 				BUG_ON(page_count(old_page) != 1);
 				BUG_ON(huge_pte_none(pte));
 				spin_lock(&mm->page_table_lock);
-				goto retry_avoidcopy;
+				ptep = huge_pte_offset(mm, address & huge_page_mask(h));
+				if (likely(pte_same(huge_ptep_get(ptep), pte)))
+					goto retry_avoidcopy;
+				/*
+				 * race occurs while re-acquiring page_table_lock, and
+				 * our job is done.
+				 */
+				return 0;
 			}
 			WARN_ON_ONCE(1);
 		}
-- 
1.7.7.1


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

[PATCH] hugetlb: clarify hugetlb_instantiation_mutex usage

[Michal Hocko]

And the follow up documentation patch:
---
>From 51bef05e4cbf97a749e6c0245114eceb21d05d61 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Mon, 21 Nov 2011 15:39:45 +0100
Subject: [PATCH] hugetlb: clarify hugetlb_instantiation_mutex usage

Let's make it clear that we cannot race with other fault handlers due to
hugetlb (global) mutex.
Also make it clear that we want to keep pte_same checks anayway to have a
transition from the global mutex easier.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/hugetlb.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f76c7ea..611cdc8 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2348,6 +2348,9 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
 
 /*
  * Hugetlb_cow() should be called with page lock of the original hugepage held.
+ * Called with hugetlb_instantiation_mutex held and pte_page locked so we
+ * cannot race with other handlers or page migration.
+ * Keep the pte_same checks anyway to make transition from the mutex easier.
  */
 static int hugetlb_cow(struct mm_struct *mm, struct vm_area_struct *vma,
 			unsigned long address, pte_t *ptep, pte_t pte,
-- 
1.7.7.1


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic