Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
to make sure the pgtable page will not be freed concurrently.
Signed-off-by: Peter Xu <[email protected]>
---
mm/pagewalk.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index 7f1c9b274906..d98564a7be57 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -302,6 +302,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
const struct mm_walk_ops *ops = walk->ops;
int err = 0;
+ hugetlb_vma_lock_read(vma);
do {
next = hugetlb_entry_end(h, addr, end);
pte = huge_pte_offset(walk->mm, addr & hmask, sz);
@@ -314,6 +315,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
if (err)
break;
} while (addr = next, addr != end);
+ hugetlb_vma_unlock_read(vma);
return err;
}
--
2.37.3
On 29.11.22 20:35, Peter Xu wrote:
> Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> to make sure the pgtable page will not be freed concurrently.
>
> Signed-off-by: Peter Xu <[email protected]>
> ---
Acked-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 11/29/22 14:35, Peter Xu wrote:
> Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> to make sure the pgtable page will not be freed concurrently.
>
> Signed-off-by: Peter Xu <[email protected]>
> ---
> mm/pagewalk.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index 7f1c9b274906..d98564a7be57 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -302,6 +302,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
> const struct mm_walk_ops *ops = walk->ops;
> int err = 0;
>
> + hugetlb_vma_lock_read(vma);
> do {
> next = hugetlb_entry_end(h, addr, end);
> pte = huge_pte_offset(walk->mm, addr & hmask, sz);
For each found pte, we will be calling mm_walk_ops->hugetlb_entry() with
the vma_lock held. I looked into the various hugetlb_entry routines, and
I am not sure about hmm_vma_walk_hugetlb_entry. It seems like it could
possibly call hmm_vma_fault -> handle_mm_fault -> hugetlb_fault. If this
can happen, then we may have an issue as hugetlb_fault will also need to
acquire the vma_lock in read mode.
I do not know the hmm code well enough to know if this may be an actual
issue?
--
Mike Kravetz
On 12/5/22 15:33, Mike Kravetz wrote:
> On 11/29/22 14:35, Peter Xu wrote:
>> Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
>> to make sure the pgtable page will not be freed concurrently.
>>
>> Signed-off-by: Peter Xu <[email protected]>
>> ---
>> mm/pagewalk.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
>> index 7f1c9b274906..d98564a7be57 100644
>> --- a/mm/pagewalk.c
>> +++ b/mm/pagewalk.c
>> @@ -302,6 +302,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
>> const struct mm_walk_ops *ops = walk->ops;
>> int err = 0;
>>
>> + hugetlb_vma_lock_read(vma);
>> do {
>> next = hugetlb_entry_end(h, addr, end);
>> pte = huge_pte_offset(walk->mm, addr & hmask, sz);
>
> For each found pte, we will be calling mm_walk_ops->hugetlb_entry() with
> the vma_lock held. I looked into the various hugetlb_entry routines, and
> I am not sure about hmm_vma_walk_hugetlb_entry. It seems like it could
> possibly call hmm_vma_fault -> handle_mm_fault -> hugetlb_fault. If this
> can happen, then we may have an issue as hugetlb_fault will also need to
> acquire the vma_lock in read mode.
>
> I do not know the hmm code well enough to know if this may be an actual
> issue?
Oh, this sounds like a serious concern. If we add a new lock, and hold it
during callbacks that also need to take it, that's not going to work out,
right?
And yes, hmm_range_fault() and related things do a good job of revealing
this kind of deadlock. :)
thanks,
--
John Hubbard
NVIDIA
On Mon, Dec 05, 2022 at 03:52:51PM -0800, John Hubbard wrote:
> On 12/5/22 15:33, Mike Kravetz wrote:
> > On 11/29/22 14:35, Peter Xu wrote:
> > > Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> > > to make sure the pgtable page will not be freed concurrently.
> > >
> > > Signed-off-by: Peter Xu <[email protected]>
> > > ---
> > > mm/pagewalk.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> > > index 7f1c9b274906..d98564a7be57 100644
> > > --- a/mm/pagewalk.c
> > > +++ b/mm/pagewalk.c
> > > @@ -302,6 +302,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
> > > const struct mm_walk_ops *ops = walk->ops;
> > > int err = 0;
> > > + hugetlb_vma_lock_read(vma);
> > > do {
> > > next = hugetlb_entry_end(h, addr, end);
> > > pte = huge_pte_offset(walk->mm, addr & hmask, sz);
> >
> > For each found pte, we will be calling mm_walk_ops->hugetlb_entry() with
> > the vma_lock held. I looked into the various hugetlb_entry routines, and
> > I am not sure about hmm_vma_walk_hugetlb_entry. It seems like it could
> > possibly call hmm_vma_fault -> handle_mm_fault -> hugetlb_fault. If this
> > can happen, then we may have an issue as hugetlb_fault will also need to
> > acquire the vma_lock in read mode.
Thanks for spotting that, Mike.
I used to notice that path special but that's when I was still using RCU
locks who doesn't have the issue. Then I overlooked this one when
switchover.
> >
> > I do not know the hmm code well enough to know if this may be an actual
> > issue?
>
> Oh, this sounds like a serious concern. If we add a new lock, and hold it
> during callbacks that also need to take it, that's not going to work out,
> right?
>
> And yes, hmm_range_fault() and related things do a good job of revealing
> this kind of deadlock. :)
I've got a fixup attached. John, since this got your attention please also
have a look too in case there's further issues.
Thanks,
--
Peter Xu
On 12/06/22 11:45, Peter Xu wrote:
> On Mon, Dec 05, 2022 at 03:52:51PM -0800, John Hubbard wrote:
> > On 12/5/22 15:33, Mike Kravetz wrote:
> > > On 11/29/22 14:35, Peter Xu wrote:
> > > > Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> > > > to make sure the pgtable page will not be freed concurrently.
> > > >
> > > > Signed-off-by: Peter Xu <[email protected]>
> > > > ---
> > > > mm/pagewalk.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> > > > index 7f1c9b274906..d98564a7be57 100644
> > > > --- a/mm/pagewalk.c
> > > > +++ b/mm/pagewalk.c
> > > > @@ -302,6 +302,7 @@ static int walk_hugetlb_range(unsigned long addr, unsigned long end,
> > > > const struct mm_walk_ops *ops = walk->ops;
> > > > int err = 0;
> > > > + hugetlb_vma_lock_read(vma);
> > > > do {
> > > > next = hugetlb_entry_end(h, addr, end);
> > > > pte = huge_pte_offset(walk->mm, addr & hmask, sz);
> > >
> > > For each found pte, we will be calling mm_walk_ops->hugetlb_entry() with
> > > the vma_lock held. I looked into the various hugetlb_entry routines, and
> > > I am not sure about hmm_vma_walk_hugetlb_entry. It seems like it could
> > > possibly call hmm_vma_fault -> handle_mm_fault -> hugetlb_fault. If this
> > > can happen, then we may have an issue as hugetlb_fault will also need to
> > > acquire the vma_lock in read mode.
>
> Thanks for spotting that, Mike.
>
> I used to notice that path special but that's when I was still using RCU
> locks who doesn't have the issue. Then I overlooked this one when
> switchover.
>
> > >
> > > I do not know the hmm code well enough to know if this may be an actual
> > > issue?
> >
> > Oh, this sounds like a serious concern. If we add a new lock, and hold it
> > during callbacks that also need to take it, that's not going to work out,
> > right?
> >
> > And yes, hmm_range_fault() and related things do a good job of revealing
> > this kind of deadlock. :)
>
> I've got a fixup attached. John, since this got your attention please also
> have a look too in case there's further issues.
>
> Thanks,
>
> --
> Peter Xu
Thanks Peter. I am good with the fixup. When combined with original,
Reviewed-by: Mike Kravetz <[email protected]>
--
Mike Kravetz
> From 9ad1e65a31f51a0dc687cd9d6083b9e920d2da61 Mon Sep 17 00:00:00 2001
> From: Peter Xu <[email protected]>
> Date: Tue, 6 Dec 2022 11:38:47 -0500
> Subject: [PATCH] fixup! mm/hugetlb: Make walk_hugetlb_range() safe to pmd
> unshare
> Content-type: text/plain
>
> Signed-off-by: Peter Xu <[email protected]>
> ---
> arch/s390/mm/gmap.c | 2 ++
> fs/proc/task_mmu.c | 2 ++
> include/linux/pagewalk.h | 8 +++++++-
> mm/hmm.c | 8 +++++++-
> 4 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 8947451ae021..292a54c490d4 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2643,7 +2643,9 @@ static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
> end = start + HPAGE_SIZE - 1;
> __storage_key_init_range(start, end);
> set_bit(PG_arch_1, &page->flags);
> + hugetlb_vma_unlock_read(walk->vma);
> cond_resched();
> + hugetlb_vma_lock_read(walk->vma);
> return 0;
> }
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 89338950afd3..d7155f3bb678 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1612,7 +1612,9 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
> frame++;
> }
>
> + hugetlb_vma_unlock_read(walk->vma);
> cond_resched();
> + hugetlb_vma_lock_read(walk->vma);
>
> return err;
> }
> diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
> index 959f52e5867d..1f7c2011f6cb 100644
> --- a/include/linux/pagewalk.h
> +++ b/include/linux/pagewalk.h
> @@ -21,7 +21,13 @@ struct mm_walk;
> * depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
> * Any folded depths (where PTRS_PER_P?D is equal to 1)
> * are skipped.
> - * @hugetlb_entry: if set, called for each hugetlb entry
> + * @hugetlb_entry: if set, called for each hugetlb entry. Note that
> + * currently the hook function is protected by hugetlb
> + * vma lock to make sure pte_t* and the spinlock is valid
> + * to access. If the hook function needs to yield the
> + * thread or retake the vma lock for some reason, it
> + * needs to properly release the vma lock manually,
> + * and retake it before the function returns.
> * @test_walk: caller specific callback function to determine whether
> * we walk over the current vma or not. Returning 0 means
> * "do page table walk over the current vma", returning
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 3850fb625dda..dcd624f28bcf 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -493,8 +493,14 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,
> required_fault =
> hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, cpu_flags);
> if (required_fault) {
> + int ret;
> +
> spin_unlock(ptl);
> - return hmm_vma_fault(addr, end, required_fault, walk);
> + hugetlb_vma_unlock_read(vma);
> + /* hmm_vma_fault() can retake the vma lock */
> + ret = hmm_vma_fault(addr, end, required_fault, walk);
> + hugetlb_vma_lock_read(vma);
> + return ret;
> }
>
> pfn = pte_pfn(entry) + ((start & ~hmask) >> PAGE_SHIFT);
> --
> 2.37.3
>
On Tue, Dec 06, 2022 at 01:03:45PM -0800, John Hubbard wrote:
> On 12/6/22 08:45, Peter Xu wrote:
> > I've got a fixup attached. John, since this got your attention please also
> > have a look too in case there's further issues.
> >
>
> Well, one question: Normally, the pattern of "release_lock(A); call f();
> acquire_lock(A);" is tricky, because one must revalidate that the state
> protected by A has not changed while the lock was released. However, in
> this case, it's letting page fault handling proceed, which already
> assumes that pages might be gone, so generally that seems OK.
Yes it's tricky, but not as tricky in this case.
I hope my documentation supplemented that (in the fixup patch):
+ * @hugetlb_entry: if set, called for each hugetlb entry. Note that
+ * currently the hook function is protected by hugetlb
+ * vma lock to make sure pte_t* and the spinlock is valid
+ * to access. If the hook function needs to yield the
+ * thread or retake the vma lock for some reason, it
+ * needs to properly release the vma lock manually,
+ * and retake it before the function returns.
The vma lock here makes sure the pte_t and the pgtable spinlock being
stable. Without the lock, they're prone to be freed in parallel.
>
> However, I'm lagging behind on understanding what the vma lock actually
> protects. It seems to be a hugetlb-specific protection for concurrent
> freeing of the page tables?
Not exactly freeing, but unsharing. Mike probably has more to say. The
series is here:
https://lore.kernel.org/all/[email protected]/#t
> If so, then running a page fault handler seems safe. If there's something
> else it protects, then we might need to revalidate that after
> re-acquiring the vma lock.
Nothing to validate here. The only reason to take the vma lock is to match
with the caller who assumes the lock taken, so either it'll be released
very soon or it prepares for the next hugetlb pgtable walk (huge_pte_offset).
>
> Also, scattering hugetlb-specific locks throughout mm seems like an
> unfortuate thing, I wonder if there is a longer term plan to Not Do
> That?
So far HMM is really the only one - normally hugetlb_entry() hook is pretty
light, so not really throughout the whole mm yet. It's even not urgently
needed for the other two places calling cond_sched(), I added it mostly
just for completeness, and with the slight hope that maybe we can yield
earlier for some pmd unsharers.
But yes it's unfortunate, I just didn't come up with a good solution.
Suggestion is always welcomed.
Thanks,
--
Peter Xu
On 12/6/22 08:45, Peter Xu wrote:
> I've got a fixup attached. John, since this got your attention please also
> have a look too in case there's further issues.
>
Well, one question: Normally, the pattern of "release_lock(A); call f();
acquire_lock(A);" is tricky, because one must revalidate that the state
protected by A has not changed while the lock was released. However, in
this case, it's letting page fault handling proceed, which already
assumes that pages might be gone, so generally that seems OK.
However, I'm lagging behind on understanding what the vma lock actually
protects. It seems to be a hugetlb-specific protection for concurrent
freeing of the page tables? If so, then running a page fault handler
seems safe. If there's something else it protects, then we might need to
revalidate that after re-acquiring the vma lock.
Also, scattering hugetlb-specific locks throughout mm seems like an
unfortuate thing, I wonder if there is a longer term plan to Not Do
That?
thanks,
--
John Hubbard
NVIDIA
On 12/6/22 13:51, Peter Xu wrote:
> On Tue, Dec 06, 2022 at 01:03:45PM -0800, John Hubbard wrote:
>> On 12/6/22 08:45, Peter Xu wrote:
>>> I've got a fixup attached. John, since this got your attention please also
>>> have a look too in case there's further issues.
>>>
>>
>> Well, one question: Normally, the pattern of "release_lock(A); call f();
>> acquire_lock(A);" is tricky, because one must revalidate that the state
>> protected by A has not changed while the lock was released. However, in
>> this case, it's letting page fault handling proceed, which already
>> assumes that pages might be gone, so generally that seems OK.
>
> Yes it's tricky, but not as tricky in this case.
>
> I hope my documentation supplemented that (in the fixup patch):
>
> + * @hugetlb_entry: if set, called for each hugetlb entry. Note that
> + * currently the hook function is protected by hugetlb
> + * vma lock to make sure pte_t* and the spinlock is valid
> + * to access. If the hook function needs to yield the
So far so good...
> + * thread or retake the vma lock for some reason, it
> + * needs to properly release the vma lock manually,
> + * and retake it before the function returns.
...but you can actually delete this second sentence. It does not add
any real information--clearly, if you must drop the lock, then you must
"manually" drop the lock.
And it still ignores my original question, which I don't think I've
fully communicated. Basically, what can happen to the protected data
during the time when the lock is not held?
>
> The vma lock here makes sure the pte_t and the pgtable spinlock being
> stable. Without the lock, they're prone to be freed in parallel.
>
Yes, but think about this: if the vma lock protects against the pte
going away, then:
lock()
get a pte
unlock()
...let hmm_vma_fault() cond_resched() run...
lock()
...whoops, something else release the pte that I'd previously
retrieved.
>>
>> However, I'm lagging behind on understanding what the vma lock actually
>> protects. It seems to be a hugetlb-specific protection for concurrent
>> freeing of the page tables?
>
> Not exactly freeing, but unsharing. Mike probably has more to say. The
> series is here:
>
> https://lore.kernel.org/all/[email protected]/#t
>
>> If so, then running a page fault handler seems safe. If there's something
>> else it protects, then we might need to revalidate that after
>> re-acquiring the vma lock.
>
> Nothing to validate here. The only reason to take the vma lock is to match
> with the caller who assumes the lock taken, so either it'll be released
> very soon or it prepares for the next hugetlb pgtable walk (huge_pte_offset).
>
ummm, see above. :)
>>
>> Also, scattering hugetlb-specific locks throughout mm seems like an
>> unfortuate thing, I wonder if there is a longer term plan to Not Do
>> That?
>
> So far HMM is really the only one - normally hugetlb_entry() hook is pretty
> light, so not really throughout the whole mm yet. It's even not urgently
> needed for the other two places calling cond_sched(), I added it mostly
> just for completeness, and with the slight hope that maybe we can yield
> earlier for some pmd unsharers.
>
> But yes it's unfortunate, I just didn't come up with a good solution.
> Suggestion is always welcomed.
>
I guess it's on me to think of something cleaner, so if I do I'll pipe
up. :)
thanks,
--
John Hubbard
NVIDIA
On Tue, Dec 06, 2022 at 02:31:30PM -0800, John Hubbard wrote:
> On 12/6/22 13:51, Peter Xu wrote:
> > On Tue, Dec 06, 2022 at 01:03:45PM -0800, John Hubbard wrote:
> > > On 12/6/22 08:45, Peter Xu wrote:
> > > > I've got a fixup attached. John, since this got your attention please also
> > > > have a look too in case there's further issues.
> > > >
> > >
> > > Well, one question: Normally, the pattern of "release_lock(A); call f();
> > > acquire_lock(A);" is tricky, because one must revalidate that the state
> > > protected by A has not changed while the lock was released. However, in
> > > this case, it's letting page fault handling proceed, which already
> > > assumes that pages might be gone, so generally that seems OK.
> >
> > Yes it's tricky, but not as tricky in this case.
> >
> > I hope my documentation supplemented that (in the fixup patch):
> >
> > + * @hugetlb_entry: if set, called for each hugetlb entry. Note that
> > + * currently the hook function is protected by hugetlb
> > + * vma lock to make sure pte_t* and the spinlock is valid
> > + * to access. If the hook function needs to yield the
[1]
>
> So far so good...
>
> > + * thread or retake the vma lock for some reason, it
> > + * needs to properly release the vma lock manually,
> > + * and retake it before the function returns.
>
> ...but you can actually delete this second sentence. It does not add
> any real information--clearly, if you must drop the lock, then you must
> "manually" drop the lock.
>
> And it still ignores my original question, which I don't think I've
> fully communicated. Basically, what can happen to the protected data
> during the time when the lock is not held?
I thought I answered this one at [1] above. If not, I can extend the
answer.
What can happen is some thread can firstly unshare the pmd pgtable page
(e.g. by clearing the PUD entry in current mm), then release the pmd
pgtable page (e.g. by unmapping it) even if current thread is still
accessing it. It will cause use-after-free on the pmd pgtable page on this
thread in various ways.
One way to trigger this is when the current thread tries to take the
pgtable lock and it'll trigger warning like the call stack referenced in
the cover letter of this series:
https://lore.kernel.org/r/[email protected]
Please also feel free to read the reproducer attached in the cover letter,
it has details on how this can trigger (even though it's so hard to trigger
so I added a delay in the kernel to make it trigger). The idea should be
the same.
>
> >
> > The vma lock here makes sure the pte_t and the pgtable spinlock being
> > stable. Without the lock, they're prone to be freed in parallel.
> >
>
> Yes, but think about this: if the vma lock protects against the pte
> going away, then:
>
> lock()
> get a pte
> unlock()
>
> ...let hmm_vma_fault() cond_resched() run...
>
> lock()
> ...whoops, something else release the pte that I'd previously
> retrieved.
Here the pte_t* is never referenced again after hugetlb_entry() returned.
The loop looks like:
do {
next = hugetlb_entry_end(h, addr, end);
pte = hugetlb_walk(vma, addr & hmask, sz);
if (pte)
err = ops->hugetlb_entry(pte, hmask, addr, next, walk);
else if (ops->pte_hole)
err = ops->pte_hole(addr, next, -1, walk);
if (err)
break;
} while (addr = next, addr != end);
After hugetlb_entry() returned, we'll _never_ touch that pte again we got
from either huge_pte_offset() or hugetlb_walk() after this patchset
applied.
If we touch it, it's a potential bug as you mentioned. But we didn't.
Hope it explains.
>
> > >
> > > However, I'm lagging behind on understanding what the vma lock actually
> > > protects. It seems to be a hugetlb-specific protection for concurrent
> > > freeing of the page tables?
> >
> > Not exactly freeing, but unsharing. Mike probably has more to say. The
> > series is here:
> >
> > https://lore.kernel.org/all/[email protected]/#t
> >
> > > If so, then running a page fault handler seems safe. If there's something
> > > else it protects, then we might need to revalidate that after
> > > re-acquiring the vma lock.
> >
> > Nothing to validate here. The only reason to take the vma lock is to match
> > with the caller who assumes the lock taken, so either it'll be released
> > very soon or it prepares for the next hugetlb pgtable walk (huge_pte_offset).
> >
>
> ummm, see above. :)
>
> > >
> > > Also, scattering hugetlb-specific locks throughout mm seems like an
> > > unfortuate thing, I wonder if there is a longer term plan to Not Do
> > > That?
> >
> > So far HMM is really the only one - normally hugetlb_entry() hook is pretty
> > light, so not really throughout the whole mm yet. It's even not urgently
> > needed for the other two places calling cond_sched(), I added it mostly
> > just for completeness, and with the slight hope that maybe we can yield
> > earlier for some pmd unsharers.
> >
> > But yes it's unfortunate, I just didn't come up with a good solution.
> > Suggestion is always welcomed.
> >
>
> I guess it's on me to think of something cleaner, so if I do I'll pipe
> up. :)
That'll be very much appricated.
It's really that I don't know how to make this better, or I can rework the
series as long as it hasn't land upstream.
Thanks,
--
Peter Xu
On 12/6/22 16:07, Peter Xu wrote:
> I thought I answered this one at [1] above. If not, I can extend the
> answer.
[1] explains it, but it doesn't mention why it's safe to drop and reacquire.
...
>
> If we touch it, it's a potential bug as you mentioned. But we didn't.
>
> Hope it explains.
I think it's OK after all, because hmm_vma_fault() does revalidate after
it takes the vma lock, so that closes the loop that I was fretting over.
I was just also worried that I'd missed some other place, but it looks
like that's not the case.
So, good.
How about this incremental diff on top, as an attempt to clarify what's
going on? Or is this too much wordage? Sometimes I write too many words:
diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
index 1f7c2011f6cb..27a6df448ee5 100644
--- a/include/linux/pagewalk.h
+++ b/include/linux/pagewalk.h
@@ -21,13 +21,16 @@ struct mm_walk;
* depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
* Any folded depths (where PTRS_PER_P?D is equal to 1)
* are skipped.
- * @hugetlb_entry: if set, called for each hugetlb entry. Note that
- * currently the hook function is protected by hugetlb
- * vma lock to make sure pte_t* and the spinlock is valid
- * to access. If the hook function needs to yield the
- * thread or retake the vma lock for some reason, it
- * needs to properly release the vma lock manually,
- * and retake it before the function returns.
+ * @hugetlb_entry: if set, called for each hugetlb entry. This hook
+ * function is called with the vma lock held, in order to
+ * protect against a concurrent freeing of the pte_t* or
+ * the ptl. In some cases, the hook function needs to drop
+ * and retake the vma lock in order to avoid deadlocks
+ * while calling other functions. In such cases the hook
+ * function must either refrain from accessing the pte or
+ * ptl after dropping the vma lock, or else revalidate
+ * those items after re-acquiring the vma lock and before
+ * accessing them.
* @test_walk: caller specific callback function to determine whether
* we walk over the current vma or not. Returning 0 means
* "do page table walk over the current vma", returning
diff --git a/mm/hmm.c b/mm/hmm.c
index dcd624f28bcf..b428f2011cfd 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -497,7 +497,13 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,
spin_unlock(ptl);
hugetlb_vma_unlock_read(vma);
- /* hmm_vma_fault() can retake the vma lock */
+ /*
+ * Avoid deadlock: drop the vma lock before calling
+ * hmm_vma_fault(), which will itself potentially take and drop
+ * the vma lock. This is also correct from a protection point of
+ * view, because there is no further use here of either pte or
+ * ptl after dropping the vma lock.
+ */
ret = hmm_vma_fault(addr, end, required_fault, walk);
hugetlb_vma_lock_read(vma);
return ret;
>> I guess it's on me to think of something cleaner, so if I do I'll pipe
>> up. :)
>
> That'll be very much appricated.
>
> It's really that I don't know how to make this better, or I can rework the
> series as long as it hasn't land upstream.
>
It's always 10x easier to notice an imperfection, than it is to improve on
it. :)
thanks,
--
John Hubbard
NVIDIA
On Tue, Dec 06, 2022 at 06:38:54PM -0800, John Hubbard wrote:
> On 12/6/22 16:07, Peter Xu wrote:
> > I thought I answered this one at [1] above. If not, I can extend the
> > answer.
>
> [1] explains it, but it doesn't mention why it's safe to drop and reacquire.
>
> ...
> >
> > If we touch it, it's a potential bug as you mentioned. But we didn't.
> >
> > Hope it explains.
>
> I think it's OK after all, because hmm_vma_fault() does revalidate after
> it takes the vma lock, so that closes the loop that I was fretting over.
>
> I was just also worried that I'd missed some other place, but it looks
> like that's not the case.
>
> So, good.
>
> How about this incremental diff on top, as an attempt to clarify what's
> going on? Or is this too much wordage? Sometimes I write too many words:
Nop, that all looks good, thanks. I'll apply them in my new post.
>
>
> diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
> index 1f7c2011f6cb..27a6df448ee5 100644
> --- a/include/linux/pagewalk.h
> +++ b/include/linux/pagewalk.h
> @@ -21,13 +21,16 @@ struct mm_walk;
> * depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
> * Any folded depths (where PTRS_PER_P?D is equal to 1)
> * are skipped.
> - * @hugetlb_entry: if set, called for each hugetlb entry. Note that
> - * currently the hook function is protected by hugetlb
> - * vma lock to make sure pte_t* and the spinlock is valid
> - * to access. If the hook function needs to yield the
> - * thread or retake the vma lock for some reason, it
> - * needs to properly release the vma lock manually,
> - * and retake it before the function returns.
> + * @hugetlb_entry: if set, called for each hugetlb entry. This hook
> + * function is called with the vma lock held, in order to
> + * protect against a concurrent freeing of the pte_t* or
> + * the ptl. In some cases, the hook function needs to drop
> + * and retake the vma lock in order to avoid deadlocks
> + * while calling other functions. In such cases the hook
> + * function must either refrain from accessing the pte or
> + * ptl after dropping the vma lock, or else revalidate
> + * those items after re-acquiring the vma lock and before
> + * accessing them.
> * @test_walk: caller specific callback function to determine whether
> * we walk over the current vma or not. Returning 0 means
> * "do page table walk over the current vma", returning
> diff --git a/mm/hmm.c b/mm/hmm.c
> index dcd624f28bcf..b428f2011cfd 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -497,7 +497,13 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,
> spin_unlock(ptl);
> hugetlb_vma_unlock_read(vma);
> - /* hmm_vma_fault() can retake the vma lock */
> + /*
> + * Avoid deadlock: drop the vma lock before calling
> + * hmm_vma_fault(), which will itself potentially take and drop
> + * the vma lock. This is also correct from a protection point of
> + * view, because there is no further use here of either pte or
> + * ptl after dropping the vma lock.
> + */
> ret = hmm_vma_fault(addr, end, required_fault, walk);
> hugetlb_vma_lock_read(vma);
> return ret;
>
> > > I guess it's on me to think of something cleaner, so if I do I'll pipe
> > > up. :)
> >
> > That'll be very much appricated.
> >
> > It's really that I don't know how to make this better, or I can rework the
> > series as long as it hasn't land upstream.
> >
>
> It's always 10x easier to notice an imperfection, than it is to improve on
> it. :)
--
Peter Xu