From: markus.suvanto@gmail.com
To: David Howells <dhowells@redhat.com>, linux-afs@lists.infradead.org
Cc: Marc Dionne <marc.dionne@auristor.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/6] afs: Fixes for 3rd party-induced data corruption
Date: Thu, 09 Sep 2021 10:40:02 +0300 [thread overview]
Message-ID: <45c8affcfc62182ad6e0f816914d8834d6e9c625.camel@gmail.com> (raw)
In-Reply-To: <163111665183.283156.17200205573146438918.stgit@warthog.procyon.org.uk>
ke, 2021-09-08 kello 16:57 +0100, David Howells kirjoitti:
> Here are some fixes for AFS that can cause data corruption due to
> interaction with another client modifying data cached locally[1].
>
> (1) When d_revalidating a dentry, don't look at the inode to which it
> points. Only check the directory to which the dentry belongs. This
> was confusing things and causing the silly-rename cleanup code to
> remove the file now at the dentry of a file that got deleted.
>
> (2) Fix mmap data coherency. When a callback break is received that
> relates to a file that we have cached, the data content may have been
> changed (there are other reasons, such as the user's rights having
> been changed). However, we're checking it lazily, only on entry to
> the kernel, which doesn't happen if we have a writeable shared mapped
> page on that file.
>
> We make the kernel keep track of mmapped files and clear all PTEs
> mapping to that file as soon as the callback comes in by calling
> unmap_mapping_pages() (we don't necessarily want to zap the
> pagecache). This causes the kernel to be reentered when userspace
> tries to access the mmapped address range again - and at that point we
> can query the server and, if we need to, zap the page cache.
>
> Ideally, I would check each file at the point of notification, but
> that involves poking the server[*] - which is holding up final closure
> of the change it is making, waiting for all the clients it notified to
> reply. This could then deadlock against the server. Further,
> invalidating the pagecache might call ->launder_page(), which would
> try to write to the file, which would definitely deadlock. (AFS
> doesn't lease file access).
>
> [*] Checking to see if the file content has changed is a matter of
> comparing the current data version number, but we have to ask the
> server for that. We also need to get a new callback promise and
> we need to poke the server for that too.
>
> (3) Add some more points at which the inode is validated, since we're
> doing it lazily, notably in ->read_iter() and ->page_mkwrite(), but
> also when performing some directory operations.
>
> Ideally, checking in ->read_iter() would be done in some derivation of
> filemap_read(). If we're going to call the server to read the file,
> then we get the file status fetch as part of that.
>
> (4) The above is now causing us to make a lot more calls to afs_validate()
> to check the inode - and afs_validate() takes the RCU read lock each
> time to make a quick check (ie. afs_check_validity()). This is
> entirely for the purpose of checking cb_s_break to see if the server
> we're using reinitialised its list of callbacks - however this isn't a
> very common operation, so most of the time we're taking this
> needlessly.
>
> Add a new cell-wide counter to count the number of reinitialisations
> done by any server and check that - and only if that changes, take the
> RCU read lock and check the server list (the server list may change,
> but the cell a file is part of won't).
>
> (5) Don't update vnode->cb_s_break and ->cb_v_break inside the validity
> checking loop. The cb_lock is done with read_seqretry, so we might go
> round the loop a second time after resetting those values - and that
> could cause someone else checking validity to miss something (I
> think).
>
> Also included are patches for fixes for some bugs encountered whilst
> debugging this.
>
> (6) Fix a leak of afs_read objects and fix a leak of keys hidden by that.
>
> (7) Fix a leak of pages that couldn't be added to extend a writeback.
>
>
> The patches can be found here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes
>
> David
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=214217 [1]
>
> ---
> David Howells (6):
> afs: Fix missing put on afs_read objects and missing get on the key therein
> afs: Fix page leak
> afs: Add missing vnode validation checks
> afs: Fix incorrect triggering of sillyrename on 3rd-party invalidation
> afs: Fix mmap coherency vs 3rd-party changes
> afs: Try to avoid taking RCU read lock when checking vnode validity
>
>
> fs/afs/callback.c | 44 ++++++++++++++++++-
> fs/afs/cell.c | 2 +
> fs/afs/dir.c | 57 ++++++++----------------
> fs/afs/file.c | 83 ++++++++++++++++++++++++++++++++++-
> fs/afs/inode.c | 88 +++++++++++++++++++-------------------
> fs/afs/internal.h | 10 +++++
> fs/afs/rotate.c | 1 +
> fs/afs/server.c | 2 +
> fs/afs/super.c | 1 +
> fs/afs/write.c | 27 ++++++++++--
> include/trace/events/afs.h | 8 +++-
> mm/memory.c | 1 +
> 12 files changed, 230 insertions(+), 94 deletions(-)
>
>
Tested-By: Markus Suvanto <markus.suvanto@gmail.com>
next prev parent reply other threads:[~2021-09-09 7:40 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-08 15:57 [PATCH 0/6] afs: Fixes for 3rd party-induced data corruption David Howells
2021-09-08 15:57 ` [PATCH 1/6] afs: Fix missing put on afs_read objects and missing get on the key therein David Howells
2021-09-10 21:10 ` Marc Dionne
2021-09-08 15:57 ` [PATCH 2/6] afs: Fix page leak David Howells
2021-09-08 20:37 ` Marc Dionne
2021-09-08 15:57 ` [PATCH 3/6] afs: Add missing vnode validation checks David Howells
2021-09-08 15:58 ` [PATCH 4/6] afs: Fix incorrect triggering of sillyrename on 3rd-party invalidation David Howells
2021-09-08 15:58 ` [PATCH 5/6] afs: Fix mmap coherency vs 3rd-party changes David Howells
2021-09-08 15:58 ` [PATCH 6/6] afs: Try to avoid taking RCU read lock when checking vnode validity David Howells
2021-09-09 7:40 ` markus.suvanto [this message]
2021-09-10 21:19 ` [PATCH 7/6] afs: Fix corruption in reads at fpos 2G-4G from an OpenAFS server David Howells
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=45c8affcfc62182ad6e0f816914d8834d6e9c625.camel@gmail.com \
--to=markus.suvanto@gmail.com \
--cc=dhowells@redhat.com \
--cc=linux-afs@lists.infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marc.dionne@auristor.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).