If all pages are deleted from the mapping by memory reclaim and also
moved to the cleancache:
__delete_from_page_cache
(no shadow case)
unaccount_page_cache_page
cleancache_put_page
page_cache_delete
mapping->nrpages -= nr
(nrpages becomes 0)
We don't clean the cleancache for an inode after final file truncation
(removal).
truncate_inode_pages_final
check (nrpages || nrexceptional) is false
no truncate_inode_pages
no cleancache_invalidate_inode(mapping)
These way when reading the new file created with same inode we may get
these trash leftover pages from cleancache and see wrong data instead of
the contents of the new file.
Fix it by always doing truncate_inode_pages which is already ready for
nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
Fixes: commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache")
To: Andrew Morton <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Vasily Averin <[email protected]>
Reviewed-by: Andrey Ryabinin <[email protected]>
Signed-off-by: Pavel Tikhomirov <[email protected]>
---
mm/truncate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/truncate.c b/mm/truncate.c
index 45d68e90b703..4c56c19e76eb 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -517,9 +517,9 @@ void truncate_inode_pages_final(struct address_space *mapping)
*/
xa_lock_irq(&mapping->i_pages);
xa_unlock_irq(&mapping->i_pages);
-
- truncate_inode_pages(mapping, 0);
}
+
+ truncate_inode_pages(mapping, 0);
}
EXPORT_SYMBOL(truncate_inode_pages_final);
--
2.17.1
On Mon 12-11-18 12:57:34, Pavel Tikhomirov wrote:
> If all pages are deleted from the mapping by memory reclaim and also
> moved to the cleancache:
>
> __delete_from_page_cache
> (no shadow case)
> unaccount_page_cache_page
> cleancache_put_page
> page_cache_delete
> mapping->nrpages -= nr
> (nrpages becomes 0)
>
> We don't clean the cleancache for an inode after final file truncation
> (removal).
>
> truncate_inode_pages_final
> check (nrpages || nrexceptional) is false
> no truncate_inode_pages
> no cleancache_invalidate_inode(mapping)
>
> These way when reading the new file created with same inode we may get
> these trash leftover pages from cleancache and see wrong data instead of
> the contents of the new file.
>
> Fix it by always doing truncate_inode_pages which is already ready for
> nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
>
> Fixes: commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache")
> To: Andrew Morton <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Reviewed-by: Vasily Averin <[email protected]>
> Reviewed-by: Andrey Ryabinin <[email protected]>
> Signed-off-by: Pavel Tikhomirov <[email protected]>
> ---
> mm/truncate.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
The patch looks good but can you add a short comment before the
truncate_inode_pages() call explaining why it needs to be called always?
Something like:
/*
* Cleancache needs notification even if there are no pages or
* shadow entries...
*/
Otherwise you can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
>
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 45d68e90b703..4c56c19e76eb 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -517,9 +517,9 @@ void truncate_inode_pages_final(struct address_space *mapping)
> */
> xa_lock_irq(&mapping->i_pages);
> xa_unlock_irq(&mapping->i_pages);
> -
> - truncate_inode_pages(mapping, 0);
> }
> +
> + truncate_inode_pages(mapping, 0);
> }
> EXPORT_SYMBOL(truncate_inode_pages_final);
>
> --
> 2.17.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 11/12/18 2:31 PM, Jan Kara wrote:
> On Mon 12-11-18 12:57:34, Pavel Tikhomirov wrote:
>> If all pages are deleted from the mapping by memory reclaim and also
>> moved to the cleancache:
>>
>> __delete_from_page_cache
>> (no shadow case)
>> unaccount_page_cache_page
>> cleancache_put_page
>> page_cache_delete
>> mapping->nrpages -= nr
>> (nrpages becomes 0)
>>
>> We don't clean the cleancache for an inode after final file truncation
>> (removal).
>>
>> truncate_inode_pages_final
>> check (nrpages || nrexceptional) is false
>> no truncate_inode_pages
>> no cleancache_invalidate_inode(mapping)
>>
>> These way when reading the new file created with same inode we may get
>> these trash leftover pages from cleancache and see wrong data instead of
>> the contents of the new file.
>>
>> Fix it by always doing truncate_inode_pages which is already ready for
>> nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
>>
>> Fixes: commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache")
>> To: Andrew Morton <[email protected]>
>> Cc: Johannes Weiner <[email protected]>
>> Cc: Mel Gorman <[email protected]>
>> Cc: Jan Kara <[email protected]>
>> Cc: Matthew Wilcox <[email protected]>
>> Cc: Andi Kleen <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Reviewed-by: Vasily Averin <[email protected]>
>> Reviewed-by: Andrey Ryabinin <[email protected]>
>> Signed-off-by: Pavel Tikhomirov <[email protected]>
>> ---
>> mm/truncate.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> The patch looks good but can you add a short comment before the
> truncate_inode_pages() call explaining why it needs to be called always?
> Something like:
>
> /*
> * Cleancache needs notification even if there are no pages or
> * shadow entries...
> */
Or we can just call cleancache_invalidate_inode(mapping) on else branch,
so the code would be more self-explanatory, and also avoid
function call in no-cleancache setups, which should the most of setups.
On Mon 12-11-18 14:40:06, Andrey Ryabinin wrote:
>
>
> On 11/12/18 2:31 PM, Jan Kara wrote:
> > On Mon 12-11-18 12:57:34, Pavel Tikhomirov wrote:
> >> If all pages are deleted from the mapping by memory reclaim and also
> >> moved to the cleancache:
> >>
> >> __delete_from_page_cache
> >> (no shadow case)
> >> unaccount_page_cache_page
> >> cleancache_put_page
> >> page_cache_delete
> >> mapping->nrpages -= nr
> >> (nrpages becomes 0)
> >>
> >> We don't clean the cleancache for an inode after final file truncation
> >> (removal).
> >>
> >> truncate_inode_pages_final
> >> check (nrpages || nrexceptional) is false
> >> no truncate_inode_pages
> >> no cleancache_invalidate_inode(mapping)
> >>
> >> These way when reading the new file created with same inode we may get
> >> these trash leftover pages from cleancache and see wrong data instead of
> >> the contents of the new file.
> >>
> >> Fix it by always doing truncate_inode_pages which is already ready for
> >> nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
> >>
> >> Fixes: commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache")
> >> To: Andrew Morton <[email protected]>
> >> Cc: Johannes Weiner <[email protected]>
> >> Cc: Mel Gorman <[email protected]>
> >> Cc: Jan Kara <[email protected]>
> >> Cc: Matthew Wilcox <[email protected]>
> >> Cc: Andi Kleen <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Reviewed-by: Vasily Averin <[email protected]>
> >> Reviewed-by: Andrey Ryabinin <[email protected]>
> >> Signed-off-by: Pavel Tikhomirov <[email protected]>
> >> ---
> >> mm/truncate.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > The patch looks good but can you add a short comment before the
> > truncate_inode_pages() call explaining why it needs to be called always?
> > Something like:
> >
> > /*
> > * Cleancache needs notification even if there are no pages or
> > * shadow entries...
> > */
>
> Or we can just call cleancache_invalidate_inode(mapping) on else branch,
> so the code would be more self-explanatory, and also avoid
> function call in no-cleancache setups, which should the most of setups.
That is workable for me as well although I'd be somewhat worried that if we
have calls to inform cleancache about final inode teardown in two different
places, they can get out of sync easily. So I somewhat prefer the current
solution + comment.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
If all pages are deleted from the mapping by memory reclaim and also
moved to the cleancache:
__delete_from_page_cache
(no shadow case)
unaccount_page_cache_page
cleancache_put_page
page_cache_delete
mapping->nrpages -= nr
(nrpages becomes 0)
We don't clean the cleancache for an inode after final file truncation
(removal).
truncate_inode_pages_final
check (nrpages || nrexceptional) is false
no truncate_inode_pages
no cleancache_invalidate_inode(mapping)
These way when reading the new file created with same inode we may get
these trash leftover pages from cleancache and see wrong data instead of
the contents of the new file.
Fix it by always doing truncate_inode_pages which is already ready for
nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
v2: add comment
Fixes: commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache")
To: Andrew Morton <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Vasily Averin <[email protected]>
Reviewed-by: Andrey Ryabinin <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
Signed-off-by: Pavel Tikhomirov <[email protected]>
---
mm/truncate.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/mm/truncate.c b/mm/truncate.c
index 45d68e90b703..2c5285767ce5 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -517,9 +517,14 @@ void truncate_inode_pages_final(struct address_space *mapping)
*/
xa_lock_irq(&mapping->i_pages);
xa_unlock_irq(&mapping->i_pages);
-
- truncate_inode_pages(mapping, 0);
}
+
+ /*
+ * Cleancache needs notification even if there are no pages or shadow
+ * entries, else we will leave leftover pages in the cleancache for
+ * a deleted inode.
+ */
+ truncate_inode_pages(mapping, 0);
}
EXPORT_SYMBOL(truncate_inode_pages_final);
--
2.17.1
On Mon, 12 Nov 2018 12:31:53 +0100 Jan Kara <[email protected]> wrote:
> > mm/truncate.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> The patch looks good but can you add a short comment before the
> truncate_inode_pages() call explaining why it needs to be called always?
> Something like:
>
> /*
> * Cleancache needs notification even if there are no pages or
> * shadow entries...
> */
--- a/mm/truncate.c~mm-cleancache-fix-corruption-on-missed-inode-invalidation-fix
+++ a/mm/truncate.c
@@ -519,6 +519,10 @@ void truncate_inode_pages_final(struct a
xa_unlock_irq(&mapping->i_pages);
}
+ /*
+ * Cleancache needs notification even if there are no pages or shadow
+ * entries.
+ */
truncate_inode_pages(mapping, 0);
}
EXPORT_SYMBOL(truncate_inode_pages_final);
_
On Mon, 12 Nov 2018 12:57:34 +0300 Pavel Tikhomirov <[email protected]> wrote:
> If all pages are deleted from the mapping by memory reclaim and also
> moved to the cleancache:
>
> __delete_from_page_cache
> (no shadow case)
> unaccount_page_cache_page
> cleancache_put_page
> page_cache_delete
> mapping->nrpages -= nr
> (nrpages becomes 0)
>
> We don't clean the cleancache for an inode after final file truncation
> (removal).
>
> truncate_inode_pages_final
> check (nrpages || nrexceptional) is false
> no truncate_inode_pages
> no cleancache_invalidate_inode(mapping)
>
> These way when reading the new file created with same inode we may get
> these trash leftover pages from cleancache and see wrong data instead of
> the contents of the new file.
>
> Fix it by always doing truncate_inode_pages which is already ready for
> nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
>
Data corruption sounds serious. Shouldn't we backport this into
-stable kernels?
On 11/16/18 1:31 AM, Andrew Morton wrote:
> On Mon, 12 Nov 2018 12:57:34 +0300 Pavel Tikhomirov <[email protected]> wrote:
>
>> If all pages are deleted from the mapping by memory reclaim and also
>> moved to the cleancache:
>>
>> __delete_from_page_cache
>> (no shadow case)
>> unaccount_page_cache_page
>> cleancache_put_page
>> page_cache_delete
>> mapping->nrpages -= nr
>> (nrpages becomes 0)
>>
>> We don't clean the cleancache for an inode after final file truncation
>> (removal).
>>
>> truncate_inode_pages_final
>> check (nrpages || nrexceptional) is false
>> no truncate_inode_pages
>> no cleancache_invalidate_inode(mapping)
>>
>> These way when reading the new file created with same inode we may get
>> these trash leftover pages from cleancache and see wrong data instead of
>> the contents of the new file.
>>
>> Fix it by always doing truncate_inode_pages which is already ready for
>> nrpages == 0 && nrexceptional == 0 case and just invalidates inode.
>>
>
> Data corruption sounds serious. Shouldn't we backport this into
> -stable kernels?
Yes, it was broken in 4.14 kernel and it should affect all who uses cleancache
Fixes: commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache")