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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20181112115400.GE7175@quack2.suse.cz>
Date:   Mon, 12 Nov 2018 12:54:00 +0100
From:   Jan Kara <jack@...e.cz>
To:     Andrey Ryabinin <aryabinin@...tuozzo.com>
Cc:     Jan Kara <jack@...e.cz>,
        Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vasily Averin <vvs@...tuozzo.com>,
        Konstantin Khorenko <khorenko@...tuozzo.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Matthew Wilcox <willy@...radead.org>,
        Andi Kleen <ak@...ux.intel.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: cleancache: fix corruption on missed inode
 invalidation

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 <akpm@...ux-foundation.org>
> >> Cc: Johannes Weiner <hannes@...xchg.org>
> >> Cc: Mel Gorman <mgorman@...hsingularity.net>
> >> Cc: Jan Kara <jack@...e.cz>
> >> Cc: Matthew Wilcox <willy@...radead.org>
> >> Cc: Andi Kleen <ak@...ux.intel.com>
> >> Cc: linux-mm@...ck.org
> >> Cc: linux-kernel@...r.kernel.org
> >> Reviewed-by: Vasily Averin <vvs@...tuozzo.com>
> >> Reviewed-by: Andrey Ryabinin <aryabinin@...tuozzo.com>
> >> Signed-off-by: Pavel Tikhomirov <ptikhomirov@...tuozzo.com>
> >> ---
> >>  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 <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ