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@xxxxxxxxx>
