From d529ef83c355f97027ff85298a9709fe06216a66 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 27 Jan 2014 13:46:15 -0500 Subject: NFS: fix the handling of NFS_INO_INVALID_DATA flag in nfs_revalidate_mapping There is a possible race in how the nfs_invalidate_mapping function is handled. Currently, we go and invalidate the pages in the file and then clear NFS_INO_INVALID_DATA. The problem is that it's possible for a stale page to creep into the mapping after the page was invalidated (i.e., via readahead). If another writer comes along and sets the flag after that happens but before invalidate_inode_pages2 returns then we could clear the flag without the cache having been properly invalidated. So, we must clear the flag first and then invalidate the pages. Doing this however, opens another race: It's possible to have two concurrent read() calls that end up in nfs_revalidate_mapping at the same time. The first one clears the NFS_INO_INVALID_DATA flag and then goes to call nfs_invalidate_mapping. Just before calling that though, the other task races in, checks the flag and finds it cleared. At that point, it trusts that the mapping is good and gets the lock on the page, allowing the read() to be satisfied from the cache even though the data is no longer valid. These effects are easily manifested by running diotest3 from the LTP test suite on NFS. That program does a series of DIO writes and buffered reads. The operations are serialized and page-aligned but the existing code fails the test since it occasionally allows a read to come out of the cache incorrectly. While mixing direct and buffered I/O isn't recommended, I believe it's possible to hit this in other ways that just use buffered I/O, though that situation is much harder to reproduce. The problem is that the checking/clearing of that flag and the invalidation of the mapping really need to be atomic. Fix this by serializing concurrent invalidations with a bitlock. At the same time, we also need to allow other places that check NFS_INO_INVALID_DATA to check whether we might be in the middle of invalidating the file, so fix up a couple of places that do that to look for the new NFS_INO_INVALIDATING flag. Doing this requires us to be careful not to set the bitlock unnecessarily, so this code only does that if it believes it will be doing an invalidation. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/inode.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) (limited to 'fs/nfs/inode.c') diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index c63e15224466..0a972ee9ccc1 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -977,11 +977,11 @@ static int nfs_invalidate_mapping(struct inode *inode, struct address_space *map if (ret < 0) return ret; } - spin_lock(&inode->i_lock); - nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; - if (S_ISDIR(inode->i_mode)) + if (S_ISDIR(inode->i_mode)) { + spin_lock(&inode->i_lock); memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf)); - spin_unlock(&inode->i_lock); + spin_unlock(&inode->i_lock); + } nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE); nfs_fscache_wait_on_invalidate(inode); @@ -1008,6 +1008,7 @@ static bool nfs_mapping_need_revalidate_inode(struct inode *inode) int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping) { struct nfs_inode *nfsi = NFS_I(inode); + unsigned long *bitlock = &nfsi->flags; int ret = 0; /* swapfiles are not supposed to be shared. */ @@ -1019,12 +1020,45 @@ int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping) if (ret < 0) goto out; } + + /* + * We must clear NFS_INO_INVALID_DATA first to ensure that + * invalidations that come in while we're shooting down the mappings + * are respected. But, that leaves a race window where one revalidator + * can clear the flag, and then another checks it before the mapping + * gets invalidated. Fix that by serializing access to this part of + * the function. + * + * At the same time, we need to allow other tasks to see whether we + * might be in the middle of invalidating the pages, so we only set + * the bit lock here if it looks like we're going to be doing that. + */ + for (;;) { + ret = wait_on_bit(bitlock, NFS_INO_INVALIDATING, + nfs_wait_bit_killable, TASK_KILLABLE); + if (ret) + goto out; + if (!(nfsi->cache_validity & NFS_INO_INVALID_DATA)) + goto out; + if (!test_and_set_bit_lock(NFS_INO_INVALIDATING, bitlock)) + break; + } + + spin_lock(&inode->i_lock); if (nfsi->cache_validity & NFS_INO_INVALID_DATA) { + nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; + spin_unlock(&inode->i_lock); trace_nfs_invalidate_mapping_enter(inode); ret = nfs_invalidate_mapping(inode, mapping); trace_nfs_invalidate_mapping_exit(inode, ret); + } else { + /* something raced in and cleared the flag */ + spin_unlock(&inode->i_lock); } + clear_bit_unlock(NFS_INO_INVALIDATING, bitlock); + smp_mb__after_clear_bit(); + wake_up_bit(bitlock, NFS_INO_INVALIDATING); out: return ret; } -- cgit v1.2.3 From 17dfeb9113397a6119091a491ef7182649f0c5a9 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Tue, 28 Jan 2014 09:37:16 -0500 Subject: NFS: Fix races in nfs_revalidate_mapping Commit d529ef83c355f97027ff85298a9709fe06216a66 (NFS: fix the handling of NFS_INO_INVALID_DATA flag in nfs_revalidate_mapping) introduces a potential race, since it doesn't test the value of nfsi->cache_validity and set the bitlock in nfsi->flags atomically. Signed-off-by: Trond Myklebust Cc: Jeff Layton --- fs/nfs/inode.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) (limited to 'fs/nfs/inode.c') diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index 0a972ee9ccc1..e5070aa5f175 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -1038,24 +1038,24 @@ int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping) nfs_wait_bit_killable, TASK_KILLABLE); if (ret) goto out; - if (!(nfsi->cache_validity & NFS_INO_INVALID_DATA)) - goto out; - if (!test_and_set_bit_lock(NFS_INO_INVALIDATING, bitlock)) + spin_lock(&inode->i_lock); + if (test_bit(NFS_INO_INVALIDATING, bitlock)) { + spin_unlock(&inode->i_lock); + continue; + } + if (nfsi->cache_validity & NFS_INO_INVALID_DATA) break; - } - - spin_lock(&inode->i_lock); - if (nfsi->cache_validity & NFS_INO_INVALID_DATA) { - nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; - spin_unlock(&inode->i_lock); - trace_nfs_invalidate_mapping_enter(inode); - ret = nfs_invalidate_mapping(inode, mapping); - trace_nfs_invalidate_mapping_exit(inode, ret); - } else { - /* something raced in and cleared the flag */ spin_unlock(&inode->i_lock); + goto out; } + set_bit(NFS_INO_INVALIDATING, bitlock); + nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; + spin_unlock(&inode->i_lock); + trace_nfs_invalidate_mapping_enter(inode); + ret = nfs_invalidate_mapping(inode, mapping); + trace_nfs_invalidate_mapping_exit(inode, ret); + clear_bit_unlock(NFS_INO_INVALIDATING, bitlock); smp_mb__after_clear_bit(); wake_up_bit(bitlock, NFS_INO_INVALIDATING); -- cgit v1.2.3 From 4db72b40fdbc706f8957e9773ae73b1574b8c694 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 28 Jan 2014 13:47:46 -0500 Subject: nfs: add memory barriers around NFS_INO_INVALID_DATA and NFS_INO_INVALIDATING If the setting of NFS_INO_INVALIDATING gets reordered to before the clearing of NFS_INO_INVALID_DATA, then another task may hit a race window where both appear to be clear, even though the inode's pages are still in need of invalidation. Fix this by adding the appropriate memory barriers. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/inode.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs/nfs/inode.c') diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index e5070aa5f175..02e185168602 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -1050,6 +1050,7 @@ int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping) } set_bit(NFS_INO_INVALIDATING, bitlock); + smp_wmb(); nfsi->cache_validity &= ~NFS_INO_INVALID_DATA; spin_unlock(&inode->i_lock); trace_nfs_invalidate_mapping_enter(inode); -- cgit v1.2.3