linux-kernel - Re: [PATCH v1] mm: hugetlb: remove incorrect comment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160720092901.GA15995@www9186uo.sakura.ne.jp>
Date:	Wed, 20 Jul 2016 18:29:02 +0900
From:	Naoya Horiguchi <nao.horiguchi@...il.com>
To:	Michal Hocko <mhocko@...nel.org>
Cc:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Zhan Chen <zhanc1@...rew.cmu.edu>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] mm: hugetlb: remove incorrect comment

On Tue, Jul 19, 2016 at 11:10:53AM +0200, Michal Hocko wrote:
> On Tue 19-07-16 11:08:18, Naoya Horiguchi wrote:
> > dequeue_hwpoisoned_huge_page() can be called without page lock hold,
> > so let's remove incorrect comment.
> 
> Could you explain why the page lock is not really needed, please? Or
> what has changed that it is not needed anymore?

The reason is that dequeue_hwpoisoned_huge_page checks page_huge_active()
inside hugetlb_lock, which allows us to avoid trying to dequeue a hugepage
that are just allocated but not linked to active list yet, even without
taking page lock.

Your question makes me aware of another comment to be removed, so let me
update this.

Thanks,
Naoya Horiguchi
---
>From 7da52ca6920dcd84e3da2df619bd5242f9c3ccec Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
Date: Wed, 20 Jul 2016 18:21:33 +0900
Subject: [PATCH v2] mm: hwpoison: remove incorrect comment

dequeue_hwpoisoned_huge_page() can be called without page lock hold,
so let's remove incorrect comment.

The reason why the page lock is not really needed is that
dequeue_hwpoisoned_huge_page() checks page_huge_active() inside hugetlb_lock,
which allows us to avoid trying to dequeue a hugepage that are just allocated
but not linked to active list yet, even without taking page lock.

Reported-by: Zhan Chen <zhanc1@...rew.cmu.edu>
Signed-off-by: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
---
 mm/hugetlb.c        | 1 -
 mm/memory-failure.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index c1f3c0be150a..26f735cc7478 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4401,7 +4401,6 @@ follow_huge_pud(struct mm_struct *mm, unsigned long address,
 
 /*
  * This function is called from memory failure code.
- * Assume the caller holds page lock of the head page.
  */
 int dequeue_hwpoisoned_huge_page(struct page *hpage)
 {
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 2fcca6b0e005..7532c3a8a39c 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -741,8 +741,6 @@ static int me_huge_page(struct page *p, unsigned long pfn)
 	 * page->lru because it can be used in other hugepage operations,
 	 * such as __unmap_hugepage_range() and gather_surplus_pages().
 	 * So instead we use page_mapping() and PageAnon().
-	 * We assume that this function is called with page lock held,
-	 * so there is no race between isolation and mapping/unmapping.
 	 */
 	if (!(page_mapping(hpage) || PageAnon(hpage))) {
 		res = dequeue_hwpoisoned_huge_page(hpage);
-- 
2.7.4