diff mbox

NFS: add FATTR4_WORD1_MODE flags for cache_consistency_bitmask

Message ID 1397571892.4709.1.camel@leira.trondhjem.org (mailing list archive)
State New, archived
Headers show

Commit Message

Trond Myklebust April 15, 2014, 2:24 p.m. UTC 
On Tue, 2014-04-15 at 09:22 -0400, Trond Myklebust wrote:
> On Apr 15, 2014, at 1:02, Kinglong Mee <kinglongmee@gmail.com> wrote:
> > Next, the "stat testfile" gets data from cache,
> > because NFS_INO_INVALID_ATTR flags is cleared below.
> > 
> > Calltrace,
> > [ 4883.997254] nfs4_proc_write_setup
> > [ 4884.006885] NFS:  1365 nfs_writeback_done (status 11)
> > [ 4884.008215] nfs4_write_done
> > [ 4884.009273] nfs4_write_done_cb
> > [ 4884.010013] nfs_post_op_update_inode_force_wcc
> > [ 4884.011221] nfs_update_inode
> > [ 4884.012001] nfs_update_inode
> > [ 4884.012952] nfs_writeback_done: before nfs_should_remove_suid
> > [ 4884.014722] nfs_writeback_done: in nfs_should_remove_suid
> > [ 4884.016549] nfs4_close_done
> > [ 4884.017614] nfs_refresh_inode
> > [ 4884.018645] nfs_update_inode
> > [ 4884.019693] nfs_update_inode
> > 
> > But, if getting status before close, the mode can be update to latest.
> 
> Argh. That is a bug in nfs_update_inode(). It is not supposed to clear NFS_INO_INVALID_ATTR if nfs_fattr does not contain a complete set of attributes.
> 
> Thanks for testing, Kinglong. This is extremely helpful...

Can you please see if the following patch fixes the above issue?
8<--------------------------------------------------------------
From 12c9c63a005889d70fbcbdb746cd7e7d127b0f01 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <trond.myklebust@primarydata.com>
Date: Tue, 15 Apr 2014 10:07:57 -0400
Subject: [PATCH] NFS: Don't declare inode uptodate unless all attributes were
 checked

Fix a bug, whereby nfs_update_inode() was declaring the inode to be
up to date despite not having checked all the attributes.
The bug occurs because the temporary variable in which we cache
the validity information is 'sanitised' before reapplying to
nfsi->cache_validity.

Reported-by: Kinglong Mee <kinglongmee@gmail.com>
Cc: stable@vger.kernel.org # 3.5+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/inode.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Kinglong Mee April 16, 2014, 2:50 a.m. UTC | #1 
On 2014/4/15 22:24, Trond Myklebust wrote:
> On Tue, 2014-04-15 at 09:22 -0400, Trond Myklebust wrote:
>> On Apr 15, 2014, at 1:02, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>> Next, the "stat testfile" gets data from cache,
>>> because NFS_INO_INVALID_ATTR flags is cleared below.
>>>
>>> Calltrace,
>>> [ 4883.997254] nfs4_proc_write_setup
>>> [ 4884.006885] NFS:  1365 nfs_writeback_done (status 11)
>>> [ 4884.008215] nfs4_write_done
>>> [ 4884.009273] nfs4_write_done_cb
>>> [ 4884.010013] nfs_post_op_update_inode_force_wcc
>>> [ 4884.011221] nfs_update_inode
>>> [ 4884.012001] nfs_update_inode
>>> [ 4884.012952] nfs_writeback_done: before nfs_should_remove_suid
>>> [ 4884.014722] nfs_writeback_done: in nfs_should_remove_suid
>>> [ 4884.016549] nfs4_close_done
>>> [ 4884.017614] nfs_refresh_inode
>>> [ 4884.018645] nfs_update_inode
>>> [ 4884.019693] nfs_update_inode
>>>
>>> But, if getting status before close, the mode can be update to latest.
>>
>> Argh. That is a bug in nfs_update_inode(). It is not supposed to clear NFS_INO_INVALID_ATTR if nfs_fattr does not contain a complete set of attributes.
>>
>> Thanks for testing, Kinglong. This is extremely helpful...
>
> Can you please see if the following patch fixes the above issue?

Thanks Trond,
Yes, the following patch fixes the above issue.
With the two patches you have send, suid/sgid can be updated correctly now.

thanks,
Kinglong Mee

> 8<--------------------------------------------------------------
>>From 12c9c63a005889d70fbcbdb746cd7e7d127b0f01 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> Date: Tue, 15 Apr 2014 10:07:57 -0400
> Subject: [PATCH] NFS: Don't declare inode uptodate unless all attributes were
>   checked
>
> Fix a bug, whereby nfs_update_inode() was declaring the inode to be
> up to date despite not having checked all the attributes.
> The bug occurs because the temporary variable in which we cache
> the validity information is 'sanitised' before reapplying to
> nfsi->cache_validity.
>
> Reported-by: Kinglong Mee <kinglongmee@gmail.com>
> Cc: stable@vger.kernel.org # 3.5+
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>   fs/nfs/inode.c | 26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index c0f7b1d0b814..a6f9c8581228 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1581,18 +1581,20 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   			inode->i_version = fattr->change_attr;
>   		}
>   	} else if (server->caps & NFS_CAP_CHANGE_ATTR)
> -		invalid |= save_cache_validity;
> +		nfsi->cache_validity |= save_cache_validity;
>
>   	if (fattr->valid & NFS_ATTR_FATTR_MTIME) {
>   		memcpy(&inode->i_mtime, &fattr->mtime, sizeof(inode->i_mtime));
>   	} else if (server->caps & NFS_CAP_MTIME)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_REVAL_FORCED);
>
>   	if (fattr->valid & NFS_ATTR_FATTR_CTIME) {
>   		memcpy(&inode->i_ctime, &fattr->ctime, sizeof(inode->i_ctime));
>   	} else if (server->caps & NFS_CAP_CTIME)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_REVAL_FORCED);
>
>   	/* Check if our cached file size is stale */
> @@ -1614,7 +1616,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   					(long long)new_isize);
>   		}
>   	} else
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_REVAL_PAGECACHE
>   				| NFS_INO_REVAL_FORCED);
>
> @@ -1622,7 +1625,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   	if (fattr->valid & NFS_ATTR_FATTR_ATIME)
>   		memcpy(&inode->i_atime, &fattr->atime, sizeof(inode->i_atime));
>   	else if (server->caps & NFS_CAP_ATIME)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATIME
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATIME
>   				| NFS_INO_REVAL_FORCED);
>
>   	if (fattr->valid & NFS_ATTR_FATTR_MODE) {
> @@ -1633,7 +1637,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   			invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL;
>   		}
>   	} else if (server->caps & NFS_CAP_MODE)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_INVALID_ACCESS
>   				| NFS_INO_INVALID_ACL
>   				| NFS_INO_REVAL_FORCED);
> @@ -1644,7 +1649,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   			inode->i_uid = fattr->uid;
>   		}
>   	} else if (server->caps & NFS_CAP_OWNER)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_INVALID_ACCESS
>   				| NFS_INO_INVALID_ACL
>   				| NFS_INO_REVAL_FORCED);
> @@ -1655,7 +1661,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   			inode->i_gid = fattr->gid;
>   		}
>   	} else if (server->caps & NFS_CAP_OWNER_GROUP)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_INVALID_ACCESS
>   				| NFS_INO_INVALID_ACL
>   				| NFS_INO_REVAL_FORCED);
> @@ -1668,7 +1675,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>   			set_nlink(inode, fattr->nlink);
>   		}
>   	} else if (server->caps & NFS_CAP_NLINK)
> -		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
> +		nfsi->cache_validity |= save_cache_validity &
> +				(NFS_INO_INVALID_ATTR
>   				| NFS_INO_REVAL_FORCED);
>
>   	if (fattr->valid & NFS_ATTR_FATTR_SPACE_USED) {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index c0f7b1d0b814..a6f9c8581228 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1581,18 +1581,20 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			inode->i_version = fattr->change_attr;
 		}
 	} else if (server->caps & NFS_CAP_CHANGE_ATTR)
-		invalid |= save_cache_validity;
+		nfsi->cache_validity |= save_cache_validity;
 
 	if (fattr->valid & NFS_ATTR_FATTR_MTIME) {
 		memcpy(&inode->i_mtime, &fattr->mtime, sizeof(inode->i_mtime));
 	} else if (server->caps & NFS_CAP_MTIME)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_REVAL_FORCED);
 
 	if (fattr->valid & NFS_ATTR_FATTR_CTIME) {
 		memcpy(&inode->i_ctime, &fattr->ctime, sizeof(inode->i_ctime));
 	} else if (server->caps & NFS_CAP_CTIME)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_REVAL_FORCED);
 
 	/* Check if our cached file size is stale */
@@ -1614,7 +1616,8 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 					(long long)new_isize);
 		}
 	} else
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_REVAL_PAGECACHE
 				| NFS_INO_REVAL_FORCED);
 
@@ -1622,7 +1625,8 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	if (fattr->valid & NFS_ATTR_FATTR_ATIME)
 		memcpy(&inode->i_atime, &fattr->atime, sizeof(inode->i_atime));
 	else if (server->caps & NFS_CAP_ATIME)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATIME
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATIME
 				| NFS_INO_REVAL_FORCED);
 
 	if (fattr->valid & NFS_ATTR_FATTR_MODE) {
@@ -1633,7 +1637,8 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL;
 		}
 	} else if (server->caps & NFS_CAP_MODE)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_INVALID_ACCESS
 				| NFS_INO_INVALID_ACL
 				| NFS_INO_REVAL_FORCED);
@@ -1644,7 +1649,8 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			inode->i_uid = fattr->uid;
 		}
 	} else if (server->caps & NFS_CAP_OWNER)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_INVALID_ACCESS
 				| NFS_INO_INVALID_ACL
 				| NFS_INO_REVAL_FORCED);
@@ -1655,7 +1661,8 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			inode->i_gid = fattr->gid;
 		}
 	} else if (server->caps & NFS_CAP_OWNER_GROUP)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_INVALID_ACCESS
 				| NFS_INO_INVALID_ACL
 				| NFS_INO_REVAL_FORCED);
@@ -1668,7 +1675,8 @@  static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			set_nlink(inode, fattr->nlink);
 		}
 	} else if (server->caps & NFS_CAP_NLINK)
-		invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR
+		nfsi->cache_validity |= save_cache_validity &
+				(NFS_INO_INVALID_ATTR
 				| NFS_INO_REVAL_FORCED);
 
 	if (fattr->valid & NFS_ATTR_FATTR_SPACE_USED) {