linux-kernel - Re: [PATCH v2] mm/hwpoison: set PageHWPoison after taking page lock in memory_failure

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: <20220315054935.GA3454497@hori.linux.bs1.fc.nec.co.jp>
Date:   Tue, 15 Mar 2022 05:49:36 +0000
From:   HORIGUCHI NAOYA(堀口　直也) 
        <naoya.horiguchi@....com>
To:     Miaohe Lin <linmiaohe@...wei.com>
CC:     Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Yang Shi <shy828301@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [PATCH v2] mm/hwpoison: set PageHWPoison after taking page lock
 in memory_failure_hugetlb()

On Mon, Mar 14, 2022 at 03:10:25PM +0800, Miaohe Lin wrote:
> On 2022/3/14 10:13, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <naoya.horiguchi@....com>
> > 
> > There is a race condition between memory_failure_hugetlb() and hugetlb
> > free/demotion, which causes setting PageHWPoison flag on the wrong page
> > (which was a hugetlb when memory_failure() was called, but was removed
> > or demoted when memory_failure_hugetlb() is called).  This results in
> > killing wrong processes.  So set PageHWPoison flag with holding page lock,
> 
> It seems hold page lock could not help solve this race condition as hugetlb
> page demotion is not required to hold the page lock. Could you please explain
> this a bit more?

Sorry, the last line in the paragraph need change. What prevents the current
race is hugetlb_lock, not page lock.  The page lock is here to prevent the
race with hugepage allocation (not directly related to the current issue,
but it's still necessary).

> 
> BTW:Is there some words missing or here should be 'page lock.' instead of 'page lock,' ?

I should use a period here, I'll fix it.

[...]

> > @@ -1503,24 +1502,11 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> >  	int res;
> >  	unsigned long page_flags;
> >  
> > -	if (TestSetPageHWPoison(head)) {
> > -		pr_err("Memory failure: %#lx: already hardware poisoned\n",
> > -		       pfn);
> > -		res = -EHWPOISON;
> > -		if (flags & MF_ACTION_REQUIRED)
> > -			res = kill_accessing_process(current, page_to_pfn(head), flags);
> > -		return res;
> > -	}
> > -
> > -	num_poisoned_pages_inc();
> > -
> >  	if (!(flags & MF_COUNT_INCREASED)) {
> >  		res = get_hwpoison_page(p, flags);
> >  		if (!res) {
> 
> In this (res == 0) case, hugetlb page could be dissolved via __page_handle_poison.
> But since PageHWPoison is not set yet, we can't set the PageHWPoison to the correct
> page. Think about the below code in dissolve_free_huge_page:
> 	/*
> 	 * Move PageHWPoison flag from head page to the raw
> 	 * error page, which makes any subpages rather than
> 	 * the error page reusable.
> 	 */
> 	if (PageHWPoison(head) && page != head) {
> 		SetPageHWPoison(page);
> 		ClearPageHWPoison(head);
> 	}
> 
> SetPageHWPoison won't be called for the error page. Or am I miss something?

No, you're right.  We need call page_handle_poison() instead of
__page_handle_poison().

@@ -1512,7 +1512,7 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
 			}
 			unlock_page(head);
 			res = MF_FAILED;
-			if (__page_handle_poison(p)) {
+			if (page_handle_poison(p, true, false)) {
 				page_ref_inc(p);
 				res = MF_RECOVERED;
 			}



> 
> >  			lock_page(head);
> >  			if (hwpoison_filter(p)) {
> > -				if (TestClearPageHWPoison(head))
> > -					num_poisoned_pages_dec();
> >  				unlock_page(head);
> >  				return -EOPNOTSUPP;
> >  			}
> > @@ -1553,13 +1539,16 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> >  	page_flags = head->flags;
> >  
> >  	if (hwpoison_filter(p)) {
> > -		if (TestClearPageHWPoison(head))
> > -			num_poisoned_pages_dec();
> >  		put_page(p);
> >  		res = -EOPNOTSUPP;
> >  		goto out;
> >  	}
> >  
> > +	if (TestSetPageHWPoison(head))
> > +		goto already_hwpoisoned;
> > +
> > +	num_poisoned_pages_inc();
> > +
> >  	/*
> >  	 * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
> >  	 * simply disable it. In order to make it work properly, we need
> > @@ -1585,6 +1574,14 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
> >  out:
> >  	unlock_page(head);
> >  	return res;
> > +already_hwpoisoned:
> > +	put_page(p);
> > +	unlock_page(head);
> 
> Generally speaking, we should do unlock_page before put_page or page might be disappeared
> before we unlock the page. This should be ok when memory_failure succeeds to handle the
> page previously as it holds one extra page refcnt. But it might be problematic when
> memory_failure failed to handle the page last time. We might be the last user here.

OK, so another code path in "if (hwpoison_filter)@ block seems to need
the same change in the order.

Thanks,
Naoya Horiguchi