Re: [External] [PATCH 7/8] hugetlb: make free_huge_page irq safe
On 3/29/21 12:49 AM, Michal Hocko wrote:
> On Sat 27-03-21 15:06:36, Muchun Song wrote:
>> On Thu, Mar 25, 2021 at 8:29 AM Mike Kravetz wrote:
>>>
>>> Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in
>>> non-task context") was added to address the issue of free_huge_page
>>> being called from irq context. That commit hands off free_huge_page
>>> processing to a workqueue if !in_task. However, as seen in [1] this
>>> does not cover all cases. Instead, make the locks taken in the
>>> free_huge_page irq safe.
>>>
>>> This patch does the following:
>>> - Make hugetlb_lock irq safe. This is mostly a simple process of
>>> changing spin_*lock calls to spin_*lock_irq* calls.
>>> - Make subpool lock irq safe in a similar manner.
>>> - Revert the !in_task check and workqueue handoff.
>>>
>>> [1]
>>> https://lore.kernel.org/linux-mm/f1c03b05bc43a...@google.com/
>>>
>>> Signed-off-by: Mike Kravetz
>>
>> The changes are straightforward.
>>
>> Reviewed-by: Muchun Song
>>
>> Since this patchset aims to fix a real word issue. Should we add a Fixes
>> tag?
>
> Do we know since when it is possible to use hugetlb in the networking
> context? Maybe this is possible since ever but I am wondering why the
> lockdep started complaining only now. Maybe just fuzzing finally started
> using this setup which nobody does normally.
>
>From my memory and email search, this first came up with powerpc iommu here:
https://lore.kernel.org/lkml/20180905112341.21355-1-aneesh.ku...@linux.ibm.com/
Aneesh proposed a solution similar to this, but 'fixed' the issue by changing
the powerpc code.
AFAICT, the put_page/free_huge_page code path has only been 'safe' to
call from task context since it was originally written. The real
question is when was it first possible for some code to do (the last)
put_page for a hugetlbfs page from irq context? My 'guess' is that this
may have been possible for quite a while. I can imagine a dma reference
to a hugetlb page held after the user space reference goes away.
--
Mike Kravetz
Re: [External] [PATCH 7/8] hugetlb: make free_huge_page irq safe
On Sat 27-03-21 15:06:36, Muchun Song wrote:
> On Thu, Mar 25, 2021 at 8:29 AM Mike Kravetz wrote:
> >
> > Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in
> > non-task context") was added to address the issue of free_huge_page
> > being called from irq context. That commit hands off free_huge_page
> > processing to a workqueue if !in_task. However, as seen in [1] this
> > does not cover all cases. Instead, make the locks taken in the
> > free_huge_page irq safe.
> >
> > This patch does the following:
> > - Make hugetlb_lock irq safe. This is mostly a simple process of
> > changing spin_*lock calls to spin_*lock_irq* calls.
> > - Make subpool lock irq safe in a similar manner.
> > - Revert the !in_task check and workqueue handoff.
> >
> > [1]
> > https://lore.kernel.org/linux-mm/f1c03b05bc43a...@google.com/
> >
> > Signed-off-by: Mike Kravetz
>
> The changes are straightforward.
>
> Reviewed-by: Muchun Song
>
> Since this patchset aims to fix a real word issue. Should we add a Fixes
> tag?
Do we know since when it is possible to use hugetlb in the networking
context? Maybe this is possible since ever but I am wondering why the
lockdep started complaining only now. Maybe just fuzzing finally started
using this setup which nobody does normally.
--
Michal Hocko
SUSE Labs
Re: [External] [PATCH 7/8] hugetlb: make free_huge_page irq safe
On Thu, Mar 25, 2021 at 8:29 AM Mike Kravetz wrote:
>
> Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in
> non-task context") was added to address the issue of free_huge_page
> being called from irq context. That commit hands off free_huge_page
> processing to a workqueue if !in_task. However, as seen in [1] this
> does not cover all cases. Instead, make the locks taken in the
> free_huge_page irq safe.
>
> This patch does the following:
> - Make hugetlb_lock irq safe. This is mostly a simple process of
> changing spin_*lock calls to spin_*lock_irq* calls.
> - Make subpool lock irq safe in a similar manner.
> - Revert the !in_task check and workqueue handoff.
>
> [1] https://lore.kernel.org/linux-mm/f1c03b05bc43a...@google.com/
>
> Signed-off-by: Mike Kravetz
The changes are straightforward.
Reviewed-by: Muchun Song
Since this patchset aims to fix a real word issue. Should we add a Fixes
tag?
> ---
> mm/hugetlb.c| 169 +---
> mm/hugetlb_cgroup.c | 8 +--
> 2 files changed, 67 insertions(+), 110 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index a9785e73379f..e4c441b878f2 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -93,9 +93,10 @@ static inline bool subpool_is_free(struct hugepage_subpool
> *spool)
> return true;
> }
>
> -static inline void unlock_or_release_subpool(struct hugepage_subpool *spool)
> +static inline void unlock_or_release_subpool(struct hugepage_subpool *spool,
> + unsigned long irq_flags)
> {
> - spin_unlock(&spool->lock);
> + spin_unlock_irqrestore(&spool->lock, irq_flags);
>
> /* If no pages are used, and no other handles to the subpool
> * remain, give up any reservations based on minimum size and
> @@ -134,10 +135,12 @@ struct hugepage_subpool *hugepage_new_subpool(struct
> hstate *h, long max_hpages,
>
> void hugepage_put_subpool(struct hugepage_subpool *spool)
> {
> - spin_lock(&spool->lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&spool->lock, flags);
> BUG_ON(!spool->count);
> spool->count--;
> - unlock_or_release_subpool(spool);
> + unlock_or_release_subpool(spool, flags);
> }
>
> /*
> @@ -156,7 +159,7 @@ static long hugepage_subpool_get_pages(struct
> hugepage_subpool *spool,
> if (!spool)
> return ret;
>
> - spin_lock(&spool->lock);
> + spin_lock_irq(&spool->lock);
>
> if (spool->max_hpages != -1) { /* maximum size accounting */
> if ((spool->used_hpages + delta) <= spool->max_hpages)
> @@ -183,7 +186,7 @@ static long hugepage_subpool_get_pages(struct
> hugepage_subpool *spool,
> }
>
> unlock_ret:
> - spin_unlock(&spool->lock);
> + spin_unlock_irq(&spool->lock);
> return ret;
> }
>
> @@ -197,11 +200,12 @@ static long hugepage_subpool_put_pages(struct
> hugepage_subpool *spool,
>long delta)
> {
> long ret = delta;
> + unsigned long flags;
>
> if (!spool)
> return delta;
>
> - spin_lock(&spool->lock);
> + spin_lock_irqsave(&spool->lock, flags);
>
> if (spool->max_hpages != -1)/* maximum size accounting */
> spool->used_hpages -= delta;
> @@ -222,7 +226,7 @@ static long hugepage_subpool_put_pages(struct
> hugepage_subpool *spool,
> * If hugetlbfs_put_super couldn't free spool due to an outstanding
> * quota reference, free it now.
> */
> - unlock_or_release_subpool(spool);
> + unlock_or_release_subpool(spool, flags);
>
> return ret;
> }
> @@ -1401,7 +1405,7 @@ struct hstate *size_to_hstate(unsigned long size)
> return NULL;
> }
>
> -static void __free_huge_page(struct page *page)
> +void free_huge_page(struct page *page)
> {
> /*
> * Can't pass hstate in here because it is called from the
> @@ -1411,6 +1415,7 @@ static void __free_huge_page(struct page *page)
> int nid = page_to_nid(page);
> struct hugepage_subpool *spool = hugetlb_page_subpool(page);
> bool restore_reserve;
> + unsigned long flags;
>
> VM_BUG_ON_PAGE(page_count(page), page);
> VM_BUG_ON_PAGE(page_mapcount(page), page);
> @@ -1439,7 +1444,7 @@ static void __free_huge_page(struct page *page)
> restore_reserve = true;
> }
>
> - spin_lock(&hugetlb_lock);
> + spin_lock_irqsave(&hugetlb_lock, flags);
> ClearHPageMigratable(page);
> hugetlb_cgroup_uncharge_page(hstate_index(h),
> pages_per_huge_page(h), page);
> @@ -1450,66 +1455,18 @@ static void __free_huge_page(struct page *page)
>
> if (HPageTemporary(page)) {
> remove_hugetlb_page(h, page, false);
> - spin_unlock(&hugetlb_lock)
Re: [PATCH 7/8] hugetlb: make free_huge_page irq safe
On 3/25/21 4:21 AM, Michal Hocko wrote:
> On Wed 24-03-21 17:28:34, Mike Kravetz wrote:
>> Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in
>> non-task context") was added to address the issue of free_huge_page
>> being called from irq context. That commit hands off free_huge_page
>> processing to a workqueue if !in_task. However, as seen in [1] this
>> does not cover all cases. Instead, make the locks taken in the
>> free_huge_page irq safe.
>
> I would just call out the deadlock scenario here in the changelog
> rathert than torture people by forcing them to follow up on the 0day
> report. Something like the below?
> "
> "
> However this doesn't cover all the cases as pointed out by 0day bot
> lockdep report [1]
> : Possible interrupt unsafe locking scenario:
> :
> :CPU0CPU1
> :
> : lock(hugetlb_lock);
> :local_irq_disable();
> :lock(slock-AF_INET);
> :lock(hugetlb_lock);
> :
> : lock(slock-AF_INET);
>
> Shakeel has later explained that this is very likely TCP TX
> zerocopy from hugetlb pages scenario when the networking code drops a
> last reference to hugetlb page while having IRQ disabled. Hugetlb
> freeing path doesn't disable IRQ while holding hugetlb_lock so a lock
> dependency chain can lead to a deadlock.
>
Thanks. I will update changelog.
>
>> This patch does the following:
>> - Make hugetlb_lock irq safe. This is mostly a simple process of
>> changing spin_*lock calls to spin_*lock_irq* calls.
>> - Make subpool lock irq safe in a similar manner.
>> - Revert the !in_task check and workqueue handoff.
>>
>> [1] https://lore.kernel.org/linux-mm/f1c03b05bc43a...@google.com/
>>
>> Signed-off-by: Mike Kravetz
>
> Acked-by: Michal Hocko
And, thanks for looking at the series!
--
Mike Kravetz
Re: [PATCH 7/8] hugetlb: make free_huge_page irq safe
On Wed 24-03-21 17:28:34, Mike Kravetz wrote:
> Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in
> non-task context") was added to address the issue of free_huge_page
> being called from irq context. That commit hands off free_huge_page
> processing to a workqueue if !in_task. However, as seen in [1] this
> does not cover all cases. Instead, make the locks taken in the
> free_huge_page irq safe.
I would just call out the deadlock scenario here in the changelog
rathert than torture people by forcing them to follow up on the 0day
report. Something like the below?
"
"
However this doesn't cover all the cases as pointed out by 0day bot
lockdep report [1]
: Possible interrupt unsafe locking scenario:
:
:CPU0CPU1
:
: lock(hugetlb_lock);
:local_irq_disable();
:lock(slock-AF_INET);
:lock(hugetlb_lock);
:
: lock(slock-AF_INET);
Shakeel has later explained that this is very likely TCP TX
zerocopy from hugetlb pages scenario when the networking code drops a
last reference to hugetlb page while having IRQ disabled. Hugetlb
freeing path doesn't disable IRQ while holding hugetlb_lock so a lock
dependency chain can lead to a deadlock.
> This patch does the following:
> - Make hugetlb_lock irq safe. This is mostly a simple process of
> changing spin_*lock calls to spin_*lock_irq* calls.
> - Make subpool lock irq safe in a similar manner.
> - Revert the !in_task check and workqueue handoff.
>
> [1] https://lore.kernel.org/linux-mm/f1c03b05bc43a...@google.com/
>
> Signed-off-by: Mike Kravetz
Acked-by: Michal Hocko
> ---
> mm/hugetlb.c| 169 +---
> mm/hugetlb_cgroup.c | 8 +--
> 2 files changed, 67 insertions(+), 110 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index a9785e73379f..e4c441b878f2 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -93,9 +93,10 @@ static inline bool subpool_is_free(struct hugepage_subpool
> *spool)
> return true;
> }
>
> -static inline void unlock_or_release_subpool(struct hugepage_subpool *spool)
> +static inline void unlock_or_release_subpool(struct hugepage_subpool *spool,
> + unsigned long irq_flags)
> {
> - spin_unlock(&spool->lock);
> + spin_unlock_irqrestore(&spool->lock, irq_flags);
>
> /* If no pages are used, and no other handles to the subpool
>* remain, give up any reservations based on minimum size and
> @@ -134,10 +135,12 @@ struct hugepage_subpool *hugepage_new_subpool(struct
> hstate *h, long max_hpages,
>
> void hugepage_put_subpool(struct hugepage_subpool *spool)
> {
> - spin_lock(&spool->lock);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&spool->lock, flags);
> BUG_ON(!spool->count);
> spool->count--;
> - unlock_or_release_subpool(spool);
> + unlock_or_release_subpool(spool, flags);
> }
>
> /*
> @@ -156,7 +159,7 @@ static long hugepage_subpool_get_pages(struct
> hugepage_subpool *spool,
> if (!spool)
> return ret;
>
> - spin_lock(&spool->lock);
> + spin_lock_irq(&spool->lock);
>
> if (spool->max_hpages != -1) { /* maximum size accounting */
> if ((spool->used_hpages + delta) <= spool->max_hpages)
> @@ -183,7 +186,7 @@ static long hugepage_subpool_get_pages(struct
> hugepage_subpool *spool,
> }
>
> unlock_ret:
> - spin_unlock(&spool->lock);
> + spin_unlock_irq(&spool->lock);
> return ret;
> }
>
> @@ -197,11 +200,12 @@ static long hugepage_subpool_put_pages(struct
> hugepage_subpool *spool,
> long delta)
> {
> long ret = delta;
> + unsigned long flags;
>
> if (!spool)
> return delta;
>
> - spin_lock(&spool->lock);
> + spin_lock_irqsave(&spool->lock, flags);
>
> if (spool->max_hpages != -1)/* maximum size accounting */
> spool->used_hpages -= delta;
> @@ -222,7 +226,7 @@ static long hugepage_subpool_put_pages(struct
> hugepage_subpool *spool,
>* If hugetlbfs_put_super couldn't free spool due to an outstanding
>* quota reference, free it now.
>*/
> - unlock_or_release_subpool(spool);
> + unlock_or_release_subpool(spool, flags);
>
> return ret;
> }
> @@ -1401,7 +1405,7 @@ struct hstate *size_to_hstate(unsigned long size)
> return NULL;
> }
>
> -static void __free_huge_page(struct page *page)
> +void free_huge_page(struct page *page)
> {
> /*
>* Can't pass hstate in here because it is called from the
> @@ -1411,6 +1415,7 @@ static void __free_huge_page(struct page *page)
> int nid = page_to_nid(page);
> struct hugepage_subpool *spool = hugetlb_page_subpool(page);
> bool restore_reserve;
> +
[PATCH 7/8] hugetlb: make free_huge_page irq safe
Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in
non-task context") was added to address the issue of free_huge_page
being called from irq context. That commit hands off free_huge_page
processing to a workqueue if !in_task. However, as seen in [1] this
does not cover all cases. Instead, make the locks taken in the
free_huge_page irq safe.
This patch does the following:
- Make hugetlb_lock irq safe. This is mostly a simple process of
changing spin_*lock calls to spin_*lock_irq* calls.
- Make subpool lock irq safe in a similar manner.
- Revert the !in_task check and workqueue handoff.
[1] https://lore.kernel.org/linux-mm/f1c03b05bc43a...@google.com/
Signed-off-by: Mike Kravetz
---
mm/hugetlb.c| 169 +---
mm/hugetlb_cgroup.c | 8 +--
2 files changed, 67 insertions(+), 110 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a9785e73379f..e4c441b878f2 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -93,9 +93,10 @@ static inline bool subpool_is_free(struct hugepage_subpool
*spool)
return true;
}
-static inline void unlock_or_release_subpool(struct hugepage_subpool *spool)
+static inline void unlock_or_release_subpool(struct hugepage_subpool *spool,
+ unsigned long irq_flags)
{
- spin_unlock(&spool->lock);
+ spin_unlock_irqrestore(&spool->lock, irq_flags);
/* If no pages are used, and no other handles to the subpool
* remain, give up any reservations based on minimum size and
@@ -134,10 +135,12 @@ struct hugepage_subpool *hugepage_new_subpool(struct
hstate *h, long max_hpages,
void hugepage_put_subpool(struct hugepage_subpool *spool)
{
- spin_lock(&spool->lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(&spool->lock, flags);
BUG_ON(!spool->count);
spool->count--;
- unlock_or_release_subpool(spool);
+ unlock_or_release_subpool(spool, flags);
}
/*
@@ -156,7 +159,7 @@ static long hugepage_subpool_get_pages(struct
hugepage_subpool *spool,
if (!spool)
return ret;
- spin_lock(&spool->lock);
+ spin_lock_irq(&spool->lock);
if (spool->max_hpages != -1) { /* maximum size accounting */
if ((spool->used_hpages + delta) <= spool->max_hpages)
@@ -183,7 +186,7 @@ static long hugepage_subpool_get_pages(struct
hugepage_subpool *spool,
}
unlock_ret:
- spin_unlock(&spool->lock);
+ spin_unlock_irq(&spool->lock);
return ret;
}
@@ -197,11 +200,12 @@ static long hugepage_subpool_put_pages(struct
hugepage_subpool *spool,
long delta)
{
long ret = delta;
+ unsigned long flags;
if (!spool)
return delta;
- spin_lock(&spool->lock);
+ spin_lock_irqsave(&spool->lock, flags);
if (spool->max_hpages != -1)/* maximum size accounting */
spool->used_hpages -= delta;
@@ -222,7 +226,7 @@ static long hugepage_subpool_put_pages(struct
hugepage_subpool *spool,
* If hugetlbfs_put_super couldn't free spool due to an outstanding
* quota reference, free it now.
*/
- unlock_or_release_subpool(spool);
+ unlock_or_release_subpool(spool, flags);
return ret;
}
@@ -1401,7 +1405,7 @@ struct hstate *size_to_hstate(unsigned long size)
return NULL;
}
-static void __free_huge_page(struct page *page)
+void free_huge_page(struct page *page)
{
/*
* Can't pass hstate in here because it is called from the
@@ -1411,6 +1415,7 @@ static void __free_huge_page(struct page *page)
int nid = page_to_nid(page);
struct hugepage_subpool *spool = hugetlb_page_subpool(page);
bool restore_reserve;
+ unsigned long flags;
VM_BUG_ON_PAGE(page_count(page), page);
VM_BUG_ON_PAGE(page_mapcount(page), page);
@@ -1439,7 +1444,7 @@ static void __free_huge_page(struct page *page)
restore_reserve = true;
}
- spin_lock(&hugetlb_lock);
+ spin_lock_irqsave(&hugetlb_lock, flags);
ClearHPageMigratable(page);
hugetlb_cgroup_uncharge_page(hstate_index(h),
pages_per_huge_page(h), page);
@@ -1450,66 +1455,18 @@ static void __free_huge_page(struct page *page)
if (HPageTemporary(page)) {
remove_hugetlb_page(h, page, false);
- spin_unlock(&hugetlb_lock);
+ spin_unlock_irqrestore(&hugetlb_lock, flags);
update_and_free_page(h, page);
} else if (h->surplus_huge_pages_node[nid]) {
/* remove the page from active list */
remove_hugetlb_page(h, page, true);
- spin_unlock(&hugetlb_lock);
+ spin_unlock_irqrestore(&hugetlb_lock, flags);
update_and_free_page(h, page
