Re: Cache flush issue with page_mapping_file() and swap back shmem page ?
On Wed, May 27, 2020 at 08:46:22PM -0700, Hugh Dickins wrote: > Hi Jerome, > > On Wed, 27 May 2020, Jerome Glisse wrote: > > So any arch code which uses page_mapping_file() might get the wrong > > answer, this function will return NULL for a swap backed page which > > can be a shmem pages. But shmem pages can still be shared among > > multiple process (and possibly at different virtual addresses if > > mremap was use). > > > > Attached is a patch that changes page_mapping_file() to return the > > shmem mapping for swap backed shmem page. I have not tested it (no > > way for me to test all those architecture) and i spotted this while > > working on something else. So i hope someone can take a closer look. > > I'm certainly no expert on flush_dcache_page() and friends, but I'd > be very surprised if such a problem exists, yet has gone unnoticed > for so long. page_mapping_file() itself is fairly new, added when > a risk of crashing on a race with swapoff came in: but the previous > use of page_mapping() would have suffered equally if there were such > a cache flushinhg problem here. > > And I'm afraid your patch won't do anything to help if there is a > problem: very soon after shmem calls add_to_swap_cache(), it calls > shmem_delete_from_page_cache(), which sets page->mapping to NULL. > > But I can assure you that a shmem page (unlike an anon page) is never > put into swap cache while it is mapped into userspace, and never > mapped into userspace while it is still in swap cache: does that help? > You are right i missed/forgot the part where shmem is never swapcache and mapped at the same time, thus page_mapping_file() can return NULL for those as they can no longer have alias mapping. Thank you Hugh Jérôme
Re: Cache flush issue with page_mapping_file() and swap back shmem page ?
Hi Jerome, On Wed, 27 May 2020, Jerome Glisse wrote: > So any arch code which uses page_mapping_file() might get the wrong > answer, this function will return NULL for a swap backed page which > can be a shmem pages. But shmem pages can still be shared among > multiple process (and possibly at different virtual addresses if > mremap was use). > > Attached is a patch that changes page_mapping_file() to return the > shmem mapping for swap backed shmem page. I have not tested it (no > way for me to test all those architecture) and i spotted this while > working on something else. So i hope someone can take a closer look. I'm certainly no expert on flush_dcache_page() and friends, but I'd be very surprised if such a problem exists, yet has gone unnoticed for so long. page_mapping_file() itself is fairly new, added when a risk of crashing on a race with swapoff came in: but the previous use of page_mapping() would have suffered equally if there were such a cache flushinhg problem here. And I'm afraid your patch won't do anything to help if there is a problem: very soon after shmem calls add_to_swap_cache(), it calls shmem_delete_from_page_cache(), which sets page->mapping to NULL. But I can assure you that a shmem page (unlike an anon page) is never put into swap cache while it is mapped into userspace, and never mapped into userspace while it is still in swap cache: does that help? Hugh > This might be a shmem page that is in a sense a file that > can be mapped multiple times in different processes at > possibly different virtual addresses (fork + mremap). So > return the shmem mapping that will allow any arch code to > find all mappings of the page. > > Note that even if page is not anonymous then the page might > have a NULL page->mapping field if it is being truncated, > but then it is fine as each pte poiting to the page will be > remove and cache flushing should be handled properly by that > part of the code. > > Signed-off-by: Jerome Glisse > Cc: "Huang, Ying" > Cc: Michal Hocko > Cc: Mel Gorman > Cc: Russell King > Cc: Andrew Morton > Cc: Mike Rapoport > Cc: "David S. Miller" > Cc: "James E.J. Bottomley" > --- > mm/util.c | 18 +- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/mm/util.c b/mm/util.c > index 988d11e6c17c..ec8739ab0cc3 100644 > --- a/mm/util.c > +++ b/mm/util.c > @@ -685,8 +685,24 @@ EXPORT_SYMBOL(page_mapping); > */ > struct address_space *page_mapping_file(struct page *page) > { > - if (unlikely(PageSwapCache(page))) > + if (unlikely(PageSwapCache(page))) { > + /* > + * This might be a shmem page that is in a sense a file that > + * can be mapped multiple times in different processes at > + * possibly different virtual addresses (fork + mremap). So > + * return the shmem mapping that will allow any arch code to > + * find all mappings of the page. > + * > + * Note that even if page is not anonymous then the page might > + * have a NULL page->mapping field if it is being truncated, > + * but then it is fine as each pte poiting to the page will be > + * remove and cache flushing should be handled properly by that > + * part of the code. > + */ > + if (!PageAnon(page)) > + return page->mapping; > return NULL; > + } > return page_mapping(page); > } > > -- > 2.26.2
Cache flush issue with page_mapping_file() and swap back shmem page ?
So any arch code which uses page_mapping_file() might get the wrong answer, this function will return NULL for a swap backed page which can be a shmem pages. But shmem pages can still be shared among multiple process (and possibly at different virtual addresses if mremap was use). Attached is a patch that changes page_mapping_file() to return the shmem mapping for swap backed shmem page. I have not tested it (no way for me to test all those architecture) and i spotted this while working on something else. So i hope someone can take a closer look. Cheers, Jérôme >From 6c76b9f8baa87ff872f6be5a44805a74c1e07fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Glisse?= Date: Wed, 27 May 2020 20:18:59 -0400 Subject: [PATCH] mm: fix cache flush for shmem page that are swap backed. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This might be a shmem page that is in a sense a file that can be mapped multiple times in different processes at possibly different virtual addresses (fork + mremap). So return the shmem mapping that will allow any arch code to find all mappings of the page. Note that even if page is not anonymous then the page might have a NULL page->mapping field if it is being truncated, but then it is fine as each pte poiting to the page will be remove and cache flushing should be handled properly by that part of the code. Signed-off-by: Jérôme Glisse Cc: "Huang, Ying" Cc: Michal Hocko Cc: Mel Gorman Cc: Russell King Cc: Andrew Morton Cc: Mike Rapoport Cc: "David S. Miller" Cc: "James E.J. Bottomley" --- mm/util.c | 18 +- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/mm/util.c b/mm/util.c index 988d11e6c17c..ec8739ab0cc3 100644 --- a/mm/util.c +++ b/mm/util.c @@ -685,8 +685,24 @@ EXPORT_SYMBOL(page_mapping); */ struct address_space *page_mapping_file(struct page *page) { - if (unlikely(PageSwapCache(page))) + if (unlikely(PageSwapCache(page))) { + /* +* This might be a shmem page that is in a sense a file that +* can be mapped multiple times in different processes at +* possibly different virtual addresses (fork + mremap). So +* return the shmem mapping that will allow any arch code to +* find all mappings of the page. +* +* Note that even if page is not anonymous then the page might +* have a NULL page->mapping field if it is being truncated, +* but then it is fine as each pte poiting to the page will be +* remove and cache flushing should be handled properly by that +* part of the code. +*/ + if (!PageAnon(page)) + return page->mapping; return NULL; + } return page_mapping(page); } -- 2.26.2