Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-31 Thread David Hildenbrand
On 31.03.21 12:56, Aili Yao wrote: On Thu, 25 Feb 2021 12:39:30 +0100 Oscar Salvador wrote: On Thu, Feb 25, 2021 at 11:28:18AM +, HORIGUCHI NAOYA(堀口 直也) wrote: Hi Aili, I agree that this set_mce_nospec() is not expected to be called for "already hwpoisoned" page because in the reported

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-31 Thread Aili Yao
On Thu, 25 Feb 2021 12:39:30 +0100 Oscar Salvador wrote: > On Thu, Feb 25, 2021 at 11:28:18AM +, HORIGUCHI NAOYA(堀口 直也) wrote: > > Hi Aili, > > > > I agree that this set_mce_nospec() is not expected to be called for > > "already hwpoisoned" page because in the reported case the error > >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-17 Thread Aili Yao
On Tue, 16 Mar 2021 17:29:39 -0700 "Luck, Tony" wrote: > On Fri, Mar 12, 2021 at 11:48:31PM +, Luck, Tony wrote: > > Thanks to the decode of the instruction we do have the virtual address. So > > we just need > > a safe walk of pgd->p4d->pud->pmd->pte (truncated if we hit a huge page) > >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-17 Thread Aili Yao
> > > > Returning true means you stop walking when you find the first entry pointing > > to a given pfn. But there could be multiple such entries, so if MCE SRAR is > > triggered by memory access to the larger address in hwpoisoned entries, the > > returned virtual address might be wrong. > >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-17 Thread Aili Yao
> > Returning true means you stop walking when you find the first entry pointing > to a given pfn. But there could be multiple such entries, so if MCE SRAR is > triggered by memory access to the larger address in hwpoisoned entries, the > returned virtual address might be wrong. > I can't

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-16 Thread Luck, Tony
On Fri, Mar 12, 2021 at 11:48:31PM +, Luck, Tony wrote: > Thanks to the decode of the instruction we do have the virtual address. So we > just need > a safe walk of pgd->p4d->pud->pmd->pte (truncated if we hit a huge page) with > a write > of a "not-present" value. Maybe a different poison

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-16 Thread Aili Yao
> As you answered in another email, p->mce_vaddr is set only on KERNEL_COPYIN > case, > then if p->mce_vaddr is available also for generic SRAR MCE (I'm assuming > that we are > talking about issues on race between generic SRAR MCE not only for > KERNEL_COPYIN case), > that might be helpful,

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-16 Thread 堀口 直也
On Fri, Mar 12, 2021 at 11:48:31PM +, Luck, Tony wrote: > >> will memory_failure() find it and unmap it? if succeed, then the current > >> will be > >> signaled with correct vaddr and shift? > > > > That's a very good question. I didn't see a SIGBUS when I first wrote this > > code, > >

RE: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-12 Thread Luck, Tony
>> will memory_failure() find it and unmap it? if succeed, then the current >> will be >> signaled with correct vaddr and shift? > > That's a very good question. I didn't see a SIGBUS when I first wrote this > code, > hence all the p->mce_vaddr. But now I'm > a) not sure why there wasn't a

RE: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-12 Thread Luck, Tony
> Sorry to interrupt as I am really confused here: > If it's a copyin case, has the page been mapped for the current process? Yes. The kernel has the current process mapped to the low address range and code like get_user(), put_user() "simply" reaches down to those addresses (maybe not so simply,

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-11 Thread Aili Yao
On Thu, 11 Mar 2021 17:05:53 + "Luck, Tony" wrote: > > I guess that p->mce_vaddr stores the virtual address of the error here. > > If so, sending SIGBUS with the address looks enough as we do now, so why > > do you walk page table to find the error virtual address? > > p->mce_vaddr only

RE: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-11 Thread Luck, Tony
> I guess that p->mce_vaddr stores the virtual address of the error here. > If so, sending SIGBUS with the address looks enough as we do now, so why > do you walk page table to find the error virtual address? p->mce_vaddr only has the virtual address for the COPYIN case. In that code path we

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-11 Thread Aili Yao
On Thu, 11 Mar 2021 08:55:30 + HORIGUCHI NAOYA(堀口 直也) wrote: > On Wed, Mar 10, 2021 at 02:10:42PM +0800, Aili Yao wrote: > > On Fri, 5 Mar 2021 15:55:25 + > > "Luck, Tony" wrote: > > > > > > From the walk, it seems we have got the virtual address, can we just > > > > send a SIGBUS

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-11 Thread 堀口 直也
On Wed, Mar 10, 2021 at 02:10:42PM +0800, Aili Yao wrote: > On Fri, 5 Mar 2021 15:55:25 + > "Luck, Tony" wrote: > > > > From the walk, it seems we have got the virtual address, can we just send > > > a SIGBUS with it? > > > > If the walk wins the race and the pte for the poisoned page is

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-09 Thread Aili Yao
On Fri, 5 Mar 2021 15:55:25 + "Luck, Tony" wrote: > > From the walk, it seems we have got the virtual address, can we just send a > > SIGBUS with it? > > If the walk wins the race and the pte for the poisoned page is still valid, > then yes. > > But we could have: > > CPU1

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-08 Thread 堀口 直也
On Mon, Mar 08, 2021 at 06:54:02PM +, Luck, Tony wrote: > >> So it should be safe to grab and hold a mutex. See patch below. > > > > The mutex approach looks simpler and safer, so I'm fine with it. > > Thanks. Is that an "Acked-by:"? Not yet, I intended to add it after full patch is

RE: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-08 Thread Luck, Tony
>> So it should be safe to grab and hold a mutex. See patch below. > > The mutex approach looks simpler and safer, so I'm fine with it. Thanks. Is that an "Acked-by:"? >> /** >> * memory_failure - Handle memory failure of a page. >> * @pfn: Page Number of the corrupted page >> @@ -1424,12

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-07 Thread 堀口 直也
On Fri, Mar 05, 2021 at 02:11:43PM -0800, Luck, Tony wrote: > This whole page table walking patch is trying to work around the > races caused by multiple calls to memory_failure() for the same > page. > > Maybe better to just avoid the races. The comment right above > memory_failure says: > >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-05 Thread Luck, Tony
This whole page table walking patch is trying to work around the races caused by multiple calls to memory_failure() for the same page. Maybe better to just avoid the races. The comment right above memory_failure says: * Must run in process context (e.g. a work queue) with interrupts * enabled

RE: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-05 Thread Luck, Tony
> From the walk, it seems we have got the virtual address, can we just send a > SIGBUS with it? If the walk wins the race and the pte for the poisoned page is still valid, then yes. But we could have: CPU1CPU2 memory_failure sets poison bit for struct page rmap

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-04 Thread Aili Yao
On Fri, 5 Mar 2021 09:30:16 +0800 Aili Yao wrote: > On Thu, 4 Mar 2021 15:57:20 -0800 > "Luck, Tony" wrote: > > > On Thu, Mar 04, 2021 at 02:45:24PM +0800, Aili Yao wrote: > > > > > if your methods works, should it be like this? > > > > > > > > > > 1582 pteval = > > >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-04 Thread Aili Yao
On Thu, 4 Mar 2021 15:57:20 -0800 "Luck, Tony" wrote: > On Thu, Mar 04, 2021 at 02:45:24PM +0800, Aili Yao wrote: > > > > if your methods works, should it be like this? > > > > > > > > 1582 pteval = > > > > swp_entry_to_pte(make_hwpoison_entry(subpage)); > > > > 1583

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-04 Thread Luck, Tony
On Thu, Mar 04, 2021 at 02:45:24PM +0800, Aili Yao wrote: > > > if your methods works, should it be like this? > > > > > > 1582 pteval = > > > swp_entry_to_pte(make_hwpoison_entry(subpage)); > > > 1583 if (PageHuge(page)) { > > > 1584

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Aili Yao
On Thu, 4 Mar 2021 12:19:41 +0800 Aili Yao wrote: > On Thu, 4 Mar 2021 10:16:53 +0800 > Aili Yao wrote: > > > On Wed, 3 Mar 2021 15:41:35 + > > "Luck, Tony" wrote: > > > > > > For error address with sigbus, i think this is not an issue resulted by > > > > the patch i post, before my

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Aili Yao
On Thu, 4 Mar 2021 10:16:53 +0800 Aili Yao wrote: > On Wed, 3 Mar 2021 15:41:35 + > "Luck, Tony" wrote: > > > > For error address with sigbus, i think this is not an issue resulted by > > > the patch i post, before my patch, the issue is already there. > > > I don't find a realizable way

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Aili Yao
On Wed, 3 Mar 2021 15:41:35 + "Luck, Tony" wrote: > > For error address with sigbus, i think this is not an issue resulted by the > > patch i post, before my patch, the issue is already there. > > I don't find a realizable way to get the correct address for same reason > > --- we don't

RE: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Luck, Tony
> For error address with sigbus, i think this is not an issue resulted by the > patch i post, before my patch, the issue is already there. > I don't find a realizable way to get the correct address for same reason --- > we don't know whether the page mapping is there or not when > we got to

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Aili Yao
Hi tony: > On Tue, 2 Mar 2021 19:39:53 -0800 > "Luck, Tony" wrote: > > > On Fri, Feb 26, 2021 at 10:59:15AM +0800, Aili Yao wrote: > > > Hi naoya, tony: > > > > > > > > > > Idea for what we should do next ... Now that x86 is calling > > > > > memory_failure() > > > > > from user context

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Aili Yao
On Tue, 2 Mar 2021 19:39:53 -0800 "Luck, Tony" wrote: > On Fri, Feb 26, 2021 at 10:59:15AM +0800, Aili Yao wrote: > > Hi naoya, tony: > > > > > > > > Idea for what we should do next ... Now that x86 is calling > > > > memory_failure() > > > > from user context ... maybe parallel calls for

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-03 Thread Luck, Tony
On Fri, Feb 26, 2021 at 10:59:15AM +0800, Aili Yao wrote: > Hi naoya, tony: > > > > > > Idea for what we should do next ... Now that x86 is calling > > > memory_failure() > > > from user context ... maybe parallel calls for the same page should > > > be blocked until the first caller completes

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-03-02 Thread Aili Yao
On Fri, 26 Feb 2021 09:58:37 -0800 "Luck, Tony" wrote: > On Fri, Feb 26, 2021 at 10:52:50AM +0800, Aili Yao wrote: > > Hi naoya,Oscar,david: > > > > > > > We could use some negative value (error code) to report the reported > > > > case, > > > > then as you mentioned above, some callers

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-26 Thread Luck, Tony
On Fri, Feb 26, 2021 at 10:52:50AM +0800, Aili Yao wrote: > Hi naoya,Oscar,david: > > > > > We could use some negative value (error code) to report the reported case, > > > then as you mentioned above, some callers need change to handle the > > > new case, and the same is true if you use some

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread Tony Luck
On Thu, Feb 25, 2021 at 6:23 PM HORIGUCHI NAOYA(堀口 直也) wrote: > > On Thu, Feb 25, 2021 at 10:15:42AM -0800, Luck, Tony wrote: > > CPU3 reads the poison and starts along same path that CPU2 > > did. > > I think that the MCE loop happening on CPU2 and CPU3 is unexpected > and these threads should

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread Aili Yao
Hi naoya, tony: > > > > Idea for what we should do next ... Now that x86 is calling memory_failure() > > from user context ... maybe parallel calls for the same page should > > be blocked until the first caller completes so we can: > > a) know that pages are unmapped (if that happens) > > b) all

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread Aili Yao
Hi naoya,Oscar,david: > > > We could use some negative value (error code) to report the reported case, > > then as you mentioned above, some callers need change to handle the > > new case, and the same is true if you use some positive value. > > My preference is -EHWPOISON, but other options are

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread 堀口 直也
On Thu, Feb 25, 2021 at 10:15:42AM -0800, Luck, Tony wrote: > On Thu, Feb 25, 2021 at 12:38:06PM +, HORIGUCHI NAOYA(堀口 直也) wrote: > > Thank you for shedding light on this, this race looks worrisome to me. > > We call try_to_unmap() inside memory_failure(), where we find affected > > ptes by

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread Luck, Tony
On Thu, Feb 25, 2021 at 12:38:06PM +, HORIGUCHI NAOYA(堀口 直也) wrote: > Thank you for shedding light on this, this race looks worrisome to me. > We call try_to_unmap() inside memory_failure(), where we find affected > ptes by page_vma_mapped_walk() and convert into hwpoison entires in >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread 堀口 直也
On Thu, Feb 25, 2021 at 12:39:30PM +0100, Oscar Salvador wrote: > On Thu, Feb 25, 2021 at 11:28:18AM +, HORIGUCHI NAOYA(堀口 直也) wrote: > > Hi Aili, > > > > I agree that this set_mce_nospec() is not expected to be called for > > "already hwpoisoned" page because in the reported case the error >

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread Oscar Salvador
On Thu, Feb 25, 2021 at 11:28:18AM +, HORIGUCHI NAOYA(堀口 直也) wrote: > Hi Aili, > > I agree that this set_mce_nospec() is not expected to be called for > "already hwpoisoned" page because in the reported case the error > page is already contained and no need to resort changing cache mode. Out

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-25 Thread 堀口 直也
On Thu, Feb 25, 2021 at 11:43:29AM +0800, Aili Yao wrote: > On Wed, 24 Feb 2021 11:31:55 +0100 Oscar Salvador wrote: ... > > > > > > > 3.The kill_me_maybe will check the return: > > > > > > 1244 static void kill_me_maybe(struct callback_head *cb) > > > 1245 { > > > > > > 1254 if

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-24 Thread Aili Yao
On Wed, 24 Feb 2021 11:31:55 +0100 Oscar Salvador wrote: > I have some questions: > > > 1.When LCME is enabled, and there are two processes A && B running on > > different core X && Y separately, which will access one same page, then > > the page corrupted when process A access it, a MCE will

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-24 Thread Oscar Salvador
On Wed, Feb 24, 2021 at 03:16:19PM +0800, Aili Yao wrote: > When the page is already poisoned, another memory_failure() call in the > same page now return 0, meaning OK. For nested memory mce handling, this > behavior may lead real serious problem, Example: I have some questions: > 1.When LCME

Re: [PATCH] mm,hwpoison: return -EBUSY when page already poisoned

2021-02-24 Thread David Hildenbrand
On 24.02.21 08:16, Aili Yao wrote: When the page is already poisoned, another memory_failure() call in the same page now return 0, meaning OK. For nested memory mce handling, this behavior may lead real serious problem, Example: 1.When LCME is enabled, and there are two processes A && B running