Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: [PATCH 0/6] afs: Fixes for 3rd party-induced data corruption
From:   David Howells <dhowells@redhat.com>
To:     linux-afs@lists.infradead.org
Cc:     Marc Dionne <marc.dionne@auristor.com>,
        Markus Suvanto <markus.suvanto@gmail.com>, dhowells@redhat.com,
        markus.suvanto@gmail.com, Marc Dionne <marc.dionne@auristor.com>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Wed, 08 Sep 2021 16:57:31 +0100
Message-ID: <163111665183.283156.17200205573146438918.stgit@warthog.procyon.org.uk>
User-Agent: StGit/0.23
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


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(-)