| 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
| ||
|
Message-ID: <50C73CA5.3040608@huawei.com>
Date: Tue, 11 Dec 2012 22:01:09 +0800
From: Xishi Qiu <qiuxishi@...wei.com>
To: Wanpeng Li <liwanp@...ux.vnet.ibm.com>
CC: WuJianguo <wujianguo@...wei.com>, Liujiang <jiang.liu@...wei.com>,
Simon Jeons <simon.jeons@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Borislav Petkov <bp@...en8.de>,
Andi Kleen <andi@...stfloor.org>,
Fengguang Wu <fengguang.wu@...el.com>, <linux-mm@...ck.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 1/2] MCE: fix an error of mce_bad_pages statistics
On 2012/12/11 20:42, Wanpeng Li wrote:
> On Tue, Dec 11, 2012 at 08:18:27PM +0800, Xishi Qiu wrote:
>> 1) move poisoned page check at the beginning of the function.
>> 2) add page_lock to avoid unpoison clear the flag.
>>
>> Signed-off-by: Xishi Qiu <qiuxishi@...wei.com>
>> Signed-off-by: Jiang Liu <jiang.liu@...wei.com>
>> ---
>> mm/memory-failure.c | 43 ++++++++++++++++++++++---------------------
>> 1 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 8b20278..9b74983 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1419,18 +1419,17 @@ static int soft_offline_huge_page(struct page *page, int flags)
>> unsigned long pfn = page_to_pfn(page);
>> struct page *hpage = compound_head(page);
>>
>> + if (PageHWPoison(hpage)) {
>> + pr_info("soft offline: %#lx hugepage already poisoned\n", pfn);
>> + return -EBUSY;
>> + }
>> +
>> ret = get_any_page(page, pfn, flags);
>> if (ret < 0)
>> return ret;
>> if (ret == 0)
>> goto done;
>>
>> - if (PageHWPoison(hpage)) {
>> - put_page(hpage);
>> - pr_info("soft offline: %#lx hugepage already poisoned\n", pfn);
>> - return -EBUSY;
>> - }
>> -
>> /* Keep page count to indicate a given hugepage is isolated. */
>> ret = migrate_huge_page(hpage, new_page, MPOL_MF_MOVE_ALL, false,
>> MIGRATE_SYNC);
>> @@ -1441,12 +1440,14 @@ static int soft_offline_huge_page(struct page *page, int flags)
>> return ret;
>> }
>> done:
>> - if (!PageHWPoison(hpage))
>> - atomic_long_add(1 << compound_trans_order(hpage),
>> - &mce_bad_pages);
>> + /* keep elevated page count for bad page */
>> + lock_page(hpage);
>> + atomic_long_add(1 << compound_trans_order(hpage), &mce_bad_pages);
>> set_page_hwpoison_huge_page(hpage);
>> + unlock_page(hpage);
>> +
>> dequeue_hwpoisoned_huge_page(hpage);
>> - /* keep elevated page count for bad page */
>> +
>> return ret;
>> }
>>
>> @@ -1488,6 +1489,11 @@ int soft_offline_page(struct page *page, int flags)
>> }
>> }
>>
>> + if (PageHWPoison(page)) {
>> + pr_info("soft offline: %#lx page already poisoned\n", pfn);
>> + return -EBUSY;
>> + }
>> +
>> ret = get_any_page(page, pfn, flags);
>> if (ret < 0)
>> return ret;
>> @@ -1519,19 +1525,11 @@ int soft_offline_page(struct page *page, int flags)
>> return -EIO;
>> }
>>
>> - lock_page(page);
>> - wait_on_page_writeback(page);
>> -
>> /*
>> * Synchronized using the page lock with memory_failure()
>> */
>> - if (PageHWPoison(page)) {
>> - unlock_page(page);
>> - put_page(page);
>> - pr_info("soft offline: %#lx page already poisoned\n", pfn);
>> - return -EBUSY;
>> - }
>> -
>> + lock_page(page);
>> + wait_on_page_writeback(page);
>> /*
>> * Try to invalidate first. This should work for
>> * non dirty unmapped page cache pages.
>> @@ -1582,8 +1580,11 @@ int soft_offline_page(struct page *page, int flags)
>> return ret;
>>
>> done:
>> + /* keep elevated page count for bad page */
>> + lock_page(page);
>> atomic_long_add(1, &mce_bad_pages);
>> SetPageHWPoison(page);
>> - /* keep elevated page count for bad page */
>> + unlock_page(page);
>> +
>
> Hi Xishi,
>
> Why add lock_page here, the comment in function unpoison_memory tell us
> we don't need it.
>
Hi Wangpeng,
Since unpoison is only for debugging, so it's unnecessary to add the lock
here, right?
> /*
> * This test is racy because PG_hwpoison is set outside of page lock.
> * That's acceptable because that won't trigger kernel panic. Instead,
> * the PG_hwpoison page will be caught and isolated on the entrance to
> * the free buddy page pool.
> */
>
> Futhermore, Andrew didn't like a variable called "mce_bad_pages".
>
> - Why do we have a variable called "mce_bad_pages"? MCE is an x86
> concept, and this code is in mm/. Lights are flashing, bells are
> ringing and a loudspeaker is blaring "layering violation" at us!
>
Just change the name?
Thanks
Xishi Qiu
> Regards,
> Wanpeng Li
>
>> return ret;
>> }
>> --
>> 1.7.1
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@...ck.org. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
>
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists