Re: write fault on dax mapping and usage of set_pte_at.

Re: write fault on dax mapping and usage of set_pte_at.

[Jan Kara]

Hi Aneesh,

On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
> We found this while testing dax with XFS, but i guess this is true for
> other file systems too. The stack trace looks as
> 
>  [c00000000007610c] set_pte_at+0x3c/0x190
> LR [c000000000378628] insert_pfn+0x208/0x280
> Call Trace:
> [c0000002125df980] [8000000000000104] 0x8000000000000104 (unreliable)
> [c0000002125df9c0] [c000000000378488] insert_pfn+0x68/0x280
> [c0000002125dfa30] [c0000000004a5494] dax_iomap_pte_fault.isra.7+0x734/0xa40
> [c0000002125dfb50] [c000000000627250] __xfs_filemap_fault+0x280/0x2d0
> [c0000002125dfbb0] [c000000000373abc] do_wp_page+0x48c/0xa40
> [c0000002125dfc00] [c000000000379170] __handle_mm_fault+0x8d0/0x1fd0
> [c0000002125dfd00] [c00000000037a9b0] handle_mm_fault+0x140/0x250
> [c0000002125dfd40] [c000000000074bb0] __do_page_fault+0x300/0xd60
> [c0000002125dfe20] [c00000000000acf4] handle_page_fault+0x18
> 
> 
> Now that is WARN_ON in set_pte_at which is 
> 
> 	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
> 
> Multiple architecture optimize set_pte_at based on the assumption that
> we will never use set_pte_at to update a valid pte entry. This helps in
> avoid flushing tlb etc. We should be using ptep_set_access_flags for
> this.

Hum, I didn't know about this assumption and neither did lot of other
people reviewing DAX patches. Is this documented somewhere?

> I guess iomap code doesn't handle this correctly? Or am I missing
> some other ways we can end up flushing tlb?

So for RW->RO transition we use ptep_clear_flush() in dax_entry_mkclean()
so that one is certainly safe. Similarly for unmapping. The RO->RW
transition does not seem to have any TLB flush so there TLB could still
carry stale information but it's the same as with normal page faults on
invalid PTEs or with protection faults for normal pages (see e.g.
finish_mkwrite_fault()). The only thing that's remaining is a situation
when we replace a PTE with zero page with a PTE pointing to a real storage
(block allocation on protection fault). However in this case we do
unmap_mapping_pages() in dax_insert_entry() so the PTE actually gets
cleared before we install a new correct block mapping. So this case is safe
as well. Am I missing something?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: write fault on dax mapping and usage of set_pte_at.

[Aneesh Kumar K.V]

On 2/21/19 5:42 PM, Jan Kara wrote:
> Hi Aneesh,
> 
> On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
>> We found this while testing dax with XFS, but i guess this is true for
>> other file systems too. The stack trace looks as
>>
>>   [c00000000007610c] set_pte_at+0x3c/0x190
>> LR [c000000000378628] insert_pfn+0x208/0x280
>> Call Trace:
>> [c0000002125df980] [8000000000000104] 0x8000000000000104 (unreliable)
>> [c0000002125df9c0] [c000000000378488] insert_pfn+0x68/0x280
>> [c0000002125dfa30] [c0000000004a5494] dax_iomap_pte_fault.isra.7+0x734/0xa40
>> [c0000002125dfb50] [c000000000627250] __xfs_filemap_fault+0x280/0x2d0
>> [c0000002125dfbb0] [c000000000373abc] do_wp_page+0x48c/0xa40
>> [c0000002125dfc00] [c000000000379170] __handle_mm_fault+0x8d0/0x1fd0
>> [c0000002125dfd00] [c00000000037a9b0] handle_mm_fault+0x140/0x250
>> [c0000002125dfd40] [c000000000074bb0] __do_page_fault+0x300/0xd60
>> [c0000002125dfe20] [c00000000000acf4] handle_page_fault+0x18
>>
>>
>> Now that is WARN_ON in set_pte_at which is
>>
>> 	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
>>
>> Multiple architecture optimize set_pte_at based on the assumption that
>> we will never use set_pte_at to update a valid pte entry. This helps in
>> avoid flushing tlb etc. We should be using ptep_set_access_flags for
>> this.
> 
> Hum, I didn't know about this assumption and neither did lot of other
> people reviewing DAX patches. Is this documented somewhere?
> 
>> I guess iomap code doesn't handle this correctly? Or am I missing
>> some other ways we can end up flushing tlb?
> 
> So for RW->RO transition we use ptep_clear_flush() in dax_entry_mkclean()
> so that one is certainly safe. Similarly for unmapping. The RO->RW
> transition does not seem to have any TLB flush so there TLB could still
> carry stale information but it's the same as with normal page faults on
> invalid PTEs or with protection faults for normal pages (see e.g.
> finish_mkwrite_fault()). 

I am not sure i understood that. RO -> RW transition can have stale TLB 
entries with RO mapping in them. So architecture do flush TLB during the 
fault, some may not. We do have a NestMMU issue with that transition 
which requires us to do mark the pte invalid and flush TLB for that 
transition. (see commit bd5050e38aec3055ff4257ade987d808ac93b582 )

For invalid PTEs we should have a TLB entry at all.

finish_mkwrite_fault do use wp_page_reuse() which does the right thing.


>The only thing that's remaining is a situation
> when we replace a PTE with zero page with a PTE pointing to a real storage
> (block allocation on protection fault). However in this case we do
> unmap_mapping_pages() in dax_insert_entry() so the PTE actually gets
> cleared before we install a new correct block mapping. So this case is safe
> as well. Am I missing something?
> 

Do pfn_mkwrite callback need to insert the pfn details for a RO->RW 
fault type? Can't we skip that pfn insert and let finish_mkwrite_fault 
handle that pte update?

-aneesh

Re: write fault on dax mapping and usage of set_pte_at.

[Aneesh Kumar K.V]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> On 2/21/19 5:42 PM, Jan Kara wrote:
>> Hi Aneesh,
>> 
>> On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
>>> We found this while testing dax with XFS, but i guess this is true for
>>> other file systems too. The stack trace looks as
>>>
> 
>>> I guess iomap code doesn't handle this correctly? Or am I missing
>>> some other ways we can end up flushing tlb?
>> 
>> So for RW->RO transition we use ptep_clear_flush() in dax_entry_mkclean()
>> so that one is certainly safe. Similarly for unmapping. The RO->RW
>> transition does not seem to have any TLB flush so there TLB could still
>> carry stale information but it's the same as with normal page faults on
>> invalid PTEs or with protection faults for normal pages (see e.g.
>> finish_mkwrite_fault()). 
>
> I am not sure i understood that. RO -> RW transition can have stale TLB 
> entries with RO mapping in them. So architecture do flush TLB during the 
> fault, some may not. We do have a NestMMU issue with that transition 
> which requires us to do mark the pte invalid and flush TLB for that 
> transition. (see commit bd5050e38aec3055ff4257ade987d808ac93b582 )
>
> For invalid PTEs we should have a TLB entry at all.

 For invalid PTEs we should not have a TLB entry at all.

-aneesh

Re: write fault on dax mapping and usage of set_pte_at.

[Jan Kara]

On Thu 21-02-19 19:11:14, Aneesh Kumar K.V wrote:
> On 2/21/19 5:42 PM, Jan Kara wrote:
> > Hi Aneesh,
> > 
> > On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
> > > We found this while testing dax with XFS, but i guess this is true for
> > > other file systems too. The stack trace looks as
> > > 
> > >   [c00000000007610c] set_pte_at+0x3c/0x190
> > > LR [c000000000378628] insert_pfn+0x208/0x280
> > > Call Trace:
> > > [c0000002125df980] [8000000000000104] 0x8000000000000104 (unreliable)
> > > [c0000002125df9c0] [c000000000378488] insert_pfn+0x68/0x280
> > > [c0000002125dfa30] [c0000000004a5494] dax_iomap_pte_fault.isra.7+0x734/0xa40
> > > [c0000002125dfb50] [c000000000627250] __xfs_filemap_fault+0x280/0x2d0
> > > [c0000002125dfbb0] [c000000000373abc] do_wp_page+0x48c/0xa40
> > > [c0000002125dfc00] [c000000000379170] __handle_mm_fault+0x8d0/0x1fd0
> > > [c0000002125dfd00] [c00000000037a9b0] handle_mm_fault+0x140/0x250
> > > [c0000002125dfd40] [c000000000074bb0] __do_page_fault+0x300/0xd60
> > > [c0000002125dfe20] [c00000000000acf4] handle_page_fault+0x18
> > > 
> > > 
> > > Now that is WARN_ON in set_pte_at which is
> > > 
> > > 	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
> > > 
> > > Multiple architecture optimize set_pte_at based on the assumption that
> > > we will never use set_pte_at to update a valid pte entry. This helps in
> > > avoid flushing tlb etc. We should be using ptep_set_access_flags for
> > > this.
> > 
> > Hum, I didn't know about this assumption and neither did lot of other
> > people reviewing DAX patches. Is this documented somewhere?

Any answer here?

> > > I guess iomap code doesn't handle this correctly? Or am I missing
> > > some other ways we can end up flushing tlb?
> > 
> > So for RW->RO transition we use ptep_clear_flush() in dax_entry_mkclean()
> > so that one is certainly safe. Similarly for unmapping. The RO->RW
> > transition does not seem to have any TLB flush so there TLB could still
> > carry stale information but it's the same as with normal page faults on
> > invalid PTEs or with protection faults for normal pages (see e.g.
> > finish_mkwrite_fault()).
> 
> I am not sure i understood that. RO -> RW transition can have stale TLB
> entries with RO mapping in them. So architecture do flush TLB during the
> fault, some may not. We do have a NestMMU issue with that transition which
> requires us to do mark the pte invalid and flush TLB for that transition.
> (see commit bd5050e38aec3055ff4257ade987d808ac93b582 )

OK, I see.

> For invalid PTEs we should have a TLB entry at all.
> 
> finish_mkwrite_fault do use wp_page_reuse() which does the right thing.

Yes, sorry. Somehow I've misread the code.

> > The only thing that's remaining is a situation
> > when we replace a PTE with zero page with a PTE pointing to a real storage
> > (block allocation on protection fault). However in this case we do
> > unmap_mapping_pages() in dax_insert_entry() so the PTE actually gets
> > cleared before we install a new correct block mapping. So this case is safe
> > as well. Am I missing something?
> > 
> 
> Do pfn_mkwrite callback need to insert the pfn details for a RO->RW fault
> type? Can't we skip that pfn insert and let finish_mkwrite_fault handle that
> pte update?

Yes, pfn_mkwrite() must fully update the PTE as the PTE update must happen
under a lock that is private to DAX code. Using ptep_set_access_flags()
in iomap code isn't going to be simple either. I have to think whether /
how that is possible.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: write fault on dax mapping and usage of set_pte_at.

[Aneesh Kumar K.V]

Jan Kara <jack@suse.cz> writes:

> On Thu 21-02-19 19:11:14, Aneesh Kumar K.V wrote:
>> On 2/21/19 5:42 PM, Jan Kara wrote:
>> > Hi Aneesh,
>> > 
>> > On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
>> > > We found this while testing dax with XFS, but i guess this is true for
>> > > other file systems too. The stack trace looks as
>> > > 
>> > >   [c00000000007610c] set_pte_at+0x3c/0x190
>> > > LR [c000000000378628] insert_pfn+0x208/0x280
>> > > Call Trace:
>> > > [c0000002125df980] [8000000000000104] 0x8000000000000104 (unreliable)
>> > > [c0000002125df9c0] [c000000000378488] insert_pfn+0x68/0x280
>> > > [c0000002125dfa30] [c0000000004a5494] dax_iomap_pte_fault.isra.7+0x734/0xa40
>> > > [c0000002125dfb50] [c000000000627250] __xfs_filemap_fault+0x280/0x2d0
>> > > [c0000002125dfbb0] [c000000000373abc] do_wp_page+0x48c/0xa40
>> > > [c0000002125dfc00] [c000000000379170] __handle_mm_fault+0x8d0/0x1fd0
>> > > [c0000002125dfd00] [c00000000037a9b0] handle_mm_fault+0x140/0x250
>> > > [c0000002125dfd40] [c000000000074bb0] __do_page_fault+0x300/0xd60
>> > > [c0000002125dfe20] [c00000000000acf4] handle_page_fault+0x18
>> > > 
>> > > 
>> > > Now that is WARN_ON in set_pte_at which is
>> > > 
>> > > 	VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
>> > > 
>> > > Multiple architecture optimize set_pte_at based on the assumption that
>> > > we will never use set_pte_at to update a valid pte entry. This helps in
>> > > avoid flushing tlb etc. We should be using ptep_set_access_flags for
>> > > this.
>> > 
>> > Hum, I didn't know about this assumption and neither did lot of other
>> > people reviewing DAX patches. Is this documented somewhere?
>
> Any answer here?
>

Not in the form of kernel documentation. But we had related
fixes which got applied earlier

cee216a696b2004017a5ecb583366093d90b1568
56eecdb912b536a4fa97fb5bfe5a940a54d79be6

Which also imply we possibly want to document this properly.

We use:
For a RO -> RW: update ptep_set_access_flags 
From an invalid pte to valid: set_pte_at
For read/modify/write sequence we have: ptep_prot_modify_start/ptep_prot_modify_commit

ptep_set_access_flags do carry a comment  which explains it is used
when relaxing the access.

-aneesh

Re: write fault on dax mapping and usage of set_pte_at.

[Aneesh Kumar K.V]

Jan Kara <jack@suse.cz> writes:

> On Thu 21-02-19 19:11:14, Aneesh Kumar K.V wrote:
>> On 2/21/19 5:42 PM, Jan Kara wrote:
>> > Hi Aneesh,
>> > 
>> > On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
> 
>> Do pfn_mkwrite callback need to insert the pfn details for a RO->RW fault
>> type? Can't we skip that pfn insert and let finish_mkwrite_fault handle that
>> pte update?
>
> Yes, pfn_mkwrite() must fully update the PTE as the PTE update must happen
> under a lock that is private to DAX code. Using ptep_set_access_flags()
> in iomap code isn't going to be simple either. I have to think whether /
> how that is possible.

Can we use ptep_clear_flush followed by set_pte_at()?

-aneesh

Re: write fault on dax mapping and usage of set_pte_at.

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

On Thu 21-02-19 21:37:27, Aneesh Kumar K.V wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > On Thu 21-02-19 19:11:14, Aneesh Kumar K.V wrote:
> >> On 2/21/19 5:42 PM, Jan Kara wrote:
> >> > Hi Aneesh,
> >> > 
> >> > On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
> > 
> >> Do pfn_mkwrite callback need to insert the pfn details for a RO->RW fault
> >> type? Can't we skip that pfn insert and let finish_mkwrite_fault handle that
> >> pte update?
> >
> > Yes, pfn_mkwrite() must fully update the PTE as the PTE update must happen
> > under a lock that is private to DAX code. Using ptep_set_access_flags()
> > in iomap code isn't going to be simple either. I have to think whether /
> > how that is possible.
> 
> Can we use ptep_clear_flush followed by set_pte_at()?

So in the end the thing was simpler than I thought. Does attached patch fix
the warnings for you?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-mm-Fix-modifying-of-page-protection-by-insert_pfn.patch --]
[-- Type: text/x-patch, Size: 2441 bytes --]

From ab5d1bbd74bc67982203b79a3748e3784a71b589 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Fri, 1 Mar 2019 15:16:11 +0100
Subject: [PATCH] mm: Fix modifying of page protection by insert_pfn()

Aneesh has reported that PPC triggers the following warning when
excercising DAX code:

[c00000000007610c] set_pte_at+0x3c/0x190
LR [c000000000378628] insert_pfn+0x208/0x280
Call Trace:
[c0000002125df980] [8000000000000104] 0x8000000000000104 (unreliable)
[c0000002125df9c0] [c000000000378488] insert_pfn+0x68/0x280
[c0000002125dfa30] [c0000000004a5494] dax_iomap_pte_fault.isra.7+0x734/0xa40
[c0000002125dfb50] [c000000000627250] __xfs_filemap_fault+0x280/0x2d0
[c0000002125dfbb0] [c000000000373abc] do_wp_page+0x48c/0xa40
[c0000002125dfc00] [c000000000379170] __handle_mm_fault+0x8d0/0x1fd0
[c0000002125dfd00] [c00000000037a9b0] handle_mm_fault+0x140/0x250
[c0000002125dfd40] [c000000000074bb0] __do_page_fault+0x300/0xd60
[c0000002125dfe20] [c00000000000acf4] handle_page_fault+0x18

Now that is WARN_ON in set_pte_at which is

        VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));

The problem is that on some architectures set_pte_at() cannot cope with
a situation where there is already some (different) valid entry present.

Use ptep_set_access_flags() instead to modify the pfn which is built to
deal with modifying existing PTE.

Reported-by: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/memory.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index e11ca9dd823f..ebca3071278a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1546,10 +1546,12 @@ static vm_fault_t insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 				WARN_ON_ONCE(!is_zero_pfn(pte_pfn(*pte)));
 				goto out_unlock;
 			}
-			entry = *pte;
-			goto out_mkwrite;
-		} else
-			goto out_unlock;
+			entry = pte_mkyoung(*pte);
+			entry = maybe_mkwrite(pte_mkdirty(entry), vma);
+			if (ptep_set_access_flags(vma, addr, pte, entry, 1))
+				update_mmu_cache(vma, addr, pte);
+		}
+		goto out_unlock;
 	}
 
 	/* Ok, finally just insert the thing.. */
@@ -1558,7 +1560,6 @@ static vm_fault_t insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 	else
 		entry = pte_mkspecial(pfn_t_pte(pfn, prot));
 
-out_mkwrite:
 	if (mkwrite) {
 		entry = pte_mkyoung(entry);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
-- 
2.16.4

Re: write fault on dax mapping and usage of set_pte_at.

[Chandan Rajendra]

On Friday, March 1, 2019 8:19:59 PM IST Jan Kara wrote:
> On Thu 21-02-19 21:37:27, Aneesh Kumar K.V wrote:
> > Jan Kara <jack@suse.cz> writes:
> > 
> > > On Thu 21-02-19 19:11:14, Aneesh Kumar K.V wrote:
> > >> On 2/21/19 5:42 PM, Jan Kara wrote:
> > >> > Hi Aneesh,
> > >> > 
> > >> > On Thu 21-02-19 12:52:39, Aneesh Kumar K.V wrote:
> > > 
> > >> Do pfn_mkwrite callback need to insert the pfn details for a RO->RW fault
> > >> type? Can't we skip that pfn insert and let finish_mkwrite_fault handle that
> > >> pte update?
> > >
> > > Yes, pfn_mkwrite() must fully update the PTE as the PTE update must happen
> > > under a lock that is private to DAX code. Using ptep_set_access_flags()
> > > in iomap code isn't going to be simple either. I have to think whether /
> > > how that is possible.
> > 
> > Can we use ptep_clear_flush followed by set_pte_at()?
> 
> So in the end the thing was simpler than I thought. Does attached patch fix
> the warnings for you?
> 
> 								Honza
> 

Hi Jan,

Yes, the patch attached by you does indeed fix the warnings.

-- 
chandan