[PATCH 0/3] NFS Bugfixes for 2.6.20-rc5

[PATCH 0/3] NFS Bugfixes for 2.6.20-rc5

[Trond Myklebust]

The following series fixes 3 bugs in the 2.6.20-rc5 NFS client.

 - Fix Oops in rpc_call_sync()
 - Fix races in nfs_revalidate_mapping()
 - Ensure we support selinux xattrs

Cheers,
  Trond

[PATCH 1/3] NFS: Fix Oops in rpc_call_sync()

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Fix the Oops in http://bugzilla.linux-nfs.org/show_bug.cgi?id=138
We shouldn't be calling rpc_release_task() for tasks that are not active.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 include/linux/sunrpc/sched.h |    1 -
 net/sunrpc/clnt.c            |    8 +++-----
 net/sunrpc/sched.c           |    3 ++-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index 97c7616..8b6ce60 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -250,7 +250,6 @@ void		rpc_init_task(struct rpc_task *tas
 				int flags, const struct rpc_call_ops *ops,
 				void *data);
 void		rpc_put_task(struct rpc_task *);
-void		rpc_release_task(struct rpc_task *);
 void		rpc_exit_task(struct rpc_task *);
 void		rpc_release_calldata(const struct rpc_call_ops *, void *);
 void		rpc_killall_tasks(struct rpc_clnt *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index aba528b..16c9fbc 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -490,16 +490,14 @@ int rpc_call_sync(struct rpc_clnt *clnt,
 
 	/* Set up the call info struct and execute the task */
 	status = task->tk_status;
-	if (status != 0) {
-		rpc_release_task(task);
+	if (status != 0)
 		goto out;
-	}
 	atomic_inc(&task->tk_count);
 	status = rpc_execute(task);
 	if (status == 0)
 		status = task->tk_status;
-	rpc_put_task(task);
 out:
+	rpc_put_task(task);
 	rpc_restore_sigmask(&oldset);
 	return status;
 }
@@ -537,7 +535,7 @@ rpc_call_async(struct rpc_clnt *clnt, st
 	if (status == 0)
 		rpc_execute(task);
 	else
-		rpc_release_task(task);
+		rpc_put_task(task);
 
 	rpc_restore_sigmask(&oldset);		
 	return status;
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 79bc4cd..fc083f0 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -42,6 +42,7 @@ static mempool_t	*rpc_buffer_mempool __r
 static void			__rpc_default_timer(struct rpc_task *task);
 static void			rpciod_killall(void);
 static void			rpc_async_schedule(struct work_struct *);
+static void			 rpc_release_task(struct rpc_task *task);
 
 /*
  * RPC tasks sit here while waiting for conditions to improve.
@@ -896,7 +897,7 @@ void rpc_put_task(struct rpc_task *task)
 }
 EXPORT_SYMBOL(rpc_put_task);
 
-void rpc_release_task(struct rpc_task *task)
+static void rpc_release_task(struct rpc_task *task)
 {
 #ifdef RPC_DEBUG
 	BUG_ON(task->tk_magic != RPC_TASK_MAGIC_ID);

[PATCH 2/3] NFS: Fix races in nfs_revalidate_mapping()

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Prevent the call to invalidate_inode_pages2() from racing with file writes
by taking the inode->i_mutex across the page cache flush and invalidate.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/dir.c           |    2 -
 fs/nfs/inode.c         |   97 +++++++++++++++++++++++++++++++++---------------
 fs/nfs/symlink.c       |    4 +-
 include/linux/nfs_fs.h |    1 
 4 files changed, 72 insertions(+), 32 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index dee3d6c..d9ba8cb 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -532,7 +532,7 @@ static int nfs_readdir(struct file *filp
 
 	lock_kernel();
 
-	res = nfs_revalidate_mapping(inode, filp->f_mapping);
+	res = nfs_revalidate_mapping_nolock(inode, filp->f_mapping);
 	if (res < 0) {
 		unlock_kernel();
 		return res;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 63e4702..d834982 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -665,49 +665,86 @@ int nfs_revalidate_inode(struct nfs_serv
 	return __nfs_revalidate_inode(server, inode);
 }
 
+static int nfs_invalidate_mapping_nolock(struct inode *inode, struct address_space *mapping)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	
+	if (mapping->nrpages != 0) {
+		int ret = invalidate_inode_pages2(mapping);
+		if (ret < 0)
+			return ret;
+	}
+	spin_lock(&inode->i_lock);
+	nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
+	if (S_ISDIR(inode->i_mode)) {
+		memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
+		/* This ensures we revalidate child dentries */
+		nfsi->cache_change_attribute = jiffies;
+	}
+	spin_unlock(&inode->i_lock);
+	nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
+	dfprintk(PAGECACHE, "NFS: (%s/%Ld) data cache invalidated\n",
+			inode->i_sb->s_id, (long long)NFS_FILEID(inode));
+	return 0;
+}
+
+static int nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
+{
+	int ret = 0;
+
+	mutex_lock(&inode->i_mutex);
+	if (NFS_I(inode)->cache_validity & NFS_INO_INVALID_DATA) {
+		ret = nfs_sync_mapping(mapping);
+		if (ret == 0)
+			ret = nfs_invalidate_mapping_nolock(inode, mapping);
+	}
+	mutex_unlock(&inode->i_mutex);
+	return ret;
+}
+
 /**
- * nfs_revalidate_mapping - Revalidate the pagecache
+ * nfs_revalidate_mapping_nolock - Revalidate the pagecache
  * @inode - pointer to host inode
  * @mapping - pointer to mapping
  */
-int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping)
+int nfs_revalidate_mapping_nolock(struct inode *inode, struct address_space *mapping)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);
 	int ret = 0;
 
-	if (NFS_STALE(inode))
-		ret = -ESTALE;
 	if ((nfsi->cache_validity & NFS_INO_REVAL_PAGECACHE)
-			|| nfs_attribute_timeout(inode))
+			|| nfs_attribute_timeout(inode) || NFS_STALE(inode)) {
 		ret = __nfs_revalidate_inode(NFS_SERVER(inode), inode);
-	if (ret < 0)
-		goto out;
+		if (ret < 0)
+			goto out;
+	}
+	if (nfsi->cache_validity & NFS_INO_INVALID_DATA)
+		ret = nfs_invalidate_mapping_nolock(inode, mapping);
+out:
+	return ret;
+}
 
-	if (nfsi->cache_validity & NFS_INO_INVALID_DATA) {
-		if (mapping->nrpages != 0) {
-			if (S_ISREG(inode->i_mode)) {
-				ret = nfs_sync_mapping(mapping);
-				if (ret < 0)
-					goto out;
-			}
-			ret = invalidate_inode_pages2(mapping);
-			if (ret < 0)
-				goto out;
-		}
-		spin_lock(&inode->i_lock);
-		nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
-		if (S_ISDIR(inode->i_mode)) {
-			memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
-			/* This ensures we revalidate child dentries */
-			nfsi->cache_change_attribute = jiffies;
-		}
-		spin_unlock(&inode->i_lock);
+/**
+ * nfs_revalidate_mapping - Revalidate the pagecache
+ * @inode - pointer to host inode
+ * @mapping - pointer to mapping
+ *
+ * This version of the function will take the inode->i_mutex and attempt to
+ * flush out all dirty data if it needs to invalidate the page cache.
+ */
+int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+	int ret = 0;
 
-		nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
-		dfprintk(PAGECACHE, "NFS: (%s/%Ld) data cache invalidated\n",
-				inode->i_sb->s_id,
-				(long long)NFS_FILEID(inode));
+	if ((nfsi->cache_validity & NFS_INO_REVAL_PAGECACHE)
+			|| nfs_attribute_timeout(inode) || NFS_STALE(inode)) {
+		ret = __nfs_revalidate_inode(NFS_SERVER(inode), inode);
+		if (ret < 0)
+			goto out;
 	}
+	if (nfsi->cache_validity & NFS_INO_INVALID_DATA)
+		ret = nfs_invalidate_mapping(inode, mapping);
 out:
 	return ret;
 }
diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
index 6c68611..525c136 100644
--- a/fs/nfs/symlink.c
+++ b/fs/nfs/symlink.c
@@ -50,7 +50,9 @@ static void *nfs_follow_link(struct dent
 {
 	struct inode *inode = dentry->d_inode;
 	struct page *page;
-	void *err = ERR_PTR(nfs_revalidate_mapping(inode, inode->i_mapping));
+	void *err;
+
+	err = ERR_PTR(nfs_revalidate_mapping_nolock(inode, inode->i_mapping));
 	if (err)
 		goto read_failed;
 	page = read_cache_page(&inode->i_data, 0,
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 0496306..c5d4084 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -308,6 +308,7 @@ extern int nfs_attribute_timeout(struct 
 extern int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode);
 extern int __nfs_revalidate_inode(struct nfs_server *, struct inode *);
 extern int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping);
+extern int nfs_revalidate_mapping_nolock(struct inode *inode, struct address_space *mapping);
 extern int nfs_setattr(struct dentry *, struct iattr *);
 extern void nfs_setattr_update_inode(struct inode *inode, struct iattr *attr);
 extern void nfs_begin_attr_update(struct inode *);

[PATCH 3/3] NFS: Ensure we support selinux xattrs

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

Some programs (e.g. GNU "cp") are currently crashing because NFS
allows you to inquire about SELinux security attributes (vfs_getxattr()
automatically handles this), but returns -EOPNOTSUPP when they later
attempt to set the SELinux security attributes.

The following patch just ensures that we pass the request to set security
attributes on to the default SELinux handler.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/nfs3acl.c  |   13 +++++++++++++
 fs/nfs/nfs4proc.c |   14 ++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index 7322da4..5970e7b 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -4,6 +4,9 @@ #include <linux/nfs3.h>
 #include <linux/nfs_fs.h>
 #include <linux/posix_acl_xattr.h>
 #include <linux/nfsacl.h>
+#include <linux/security.h>
+#include <linux/xattr.h>
+#include <linux/fsnotify.h>
 
 #define NFSDBG_FACILITY	NFSDBG_PROC
 
@@ -82,6 +85,16 @@ int nfs3_setxattr(struct dentry *dentry,
 	struct posix_acl *acl;
 	int type, error;
 
+	/* Selinux support */
+	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
+		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
+		error = security_inode_setsecurity(inode, suffix, value,
+				size, flags);
+		if (!error)
+			fsnotify_xattr(dentry);
+		return error;
+	}
+
 	if (strcmp(name, POSIX_ACL_XATTR_ACCESS) == 0)
 		type = ACL_TYPE_ACCESS;
 	else if (strcmp(name, POSIX_ACL_XATTR_DEFAULT) == 0)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b3fd29b..ac1082b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -48,6 +48,9 @@ #include <linux/nfs_page.h>
 #include <linux/smp_lock.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
+#include <linux/security.h>
+#include <linux/xattr.h>
+#include <linux/fsnotify.h>
 
 #include "nfs4_fs.h"
 #include "delegation.h"
@@ -3547,6 +3550,17 @@ int nfs4_setxattr(struct dentry *dentry,
 {
 	struct inode *inode = dentry->d_inode;
 
+	/* Selinux support */
+	if (!strncmp(key, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
+		const char *suffix = key + XATTR_SECURITY_PREFIX_LEN;
+		int error;
+		error = security_inode_setsecurity(inode, suffix, buf,
+				buflen, flags);
+		if (!error)
+			fsnotify_xattr(dentry);
+		return error;
+	}
+
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;
 

Re: [PATCH 3/3] NFS: Ensure we support selinux xattrs

[Stephen Smalley]

On Wed, 2007-01-24 at 11:54 -0800, Trond Myklebust wrote:
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> Some programs (e.g. GNU "cp") are currently crashing because NFS
> allows you to inquire about SELinux security attributes (vfs_getxattr()
> automatically handles this), but returns -EOPNOTSUPP when they later
> attempt to set the SELinux security attributes.
> 
> The following patch just ensures that we pass the request to set security
> attributes on to the default SELinux handler.

That seems like a bug in cp, and it can happen under other circumstances
as well (mount with context= option).

I don't think you want to just set the incore inode security state on
the client side, as that won't be preserved.

> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/nfs3acl.c  |   13 +++++++++++++
>  fs/nfs/nfs4proc.c |   14 ++++++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index 7322da4..5970e7b 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -4,6 +4,9 @@ #include <linux/nfs3.h>
>  #include <linux/nfs_fs.h>
>  #include <linux/posix_acl_xattr.h>
>  #include <linux/nfsacl.h>
> +#include <linux/security.h>
> +#include <linux/xattr.h>
> +#include <linux/fsnotify.h>
>  
>  #define NFSDBG_FACILITY	NFSDBG_PROC
>  
> @@ -82,6 +85,16 @@ int nfs3_setxattr(struct dentry *dentry,
>  	struct posix_acl *acl;
>  	int type, error;
>  
> +	/* Selinux support */
> +	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> +		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> +		error = security_inode_setsecurity(inode, suffix, value,
> +				size, flags);
> +		if (!error)
> +			fsnotify_xattr(dentry);
> +		return error;
> +	}
> +
>  	if (strcmp(name, POSIX_ACL_XATTR_ACCESS) == 0)
>  		type = ACL_TYPE_ACCESS;
>  	else if (strcmp(name, POSIX_ACL_XATTR_DEFAULT) == 0)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index b3fd29b..ac1082b 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -48,6 +48,9 @@ #include <linux/nfs_page.h>
>  #include <linux/smp_lock.h>
>  #include <linux/namei.h>
>  #include <linux/mount.h>
> +#include <linux/security.h>
> +#include <linux/xattr.h>
> +#include <linux/fsnotify.h>
>  
>  #include "nfs4_fs.h"
>  #include "delegation.h"
> @@ -3547,6 +3550,17 @@ int nfs4_setxattr(struct dentry *dentry,
>  {
>  	struct inode *inode = dentry->d_inode;
>  
> +	/* Selinux support */
> +	if (!strncmp(key, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> +		const char *suffix = key + XATTR_SECURITY_PREFIX_LEN;
> +		int error;
> +		error = security_inode_setsecurity(inode, suffix, buf,
> +				buflen, flags);
> +		if (!error)
> +			fsnotify_xattr(dentry);
> +		return error;
> +	}
> +
>  	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
>  		return -EOPNOTSUPP;
>  
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 3/3] NFS: Ensure we support selinux xattrs

[Linus Torvalds]

On Wed, 24 Jan 2007, Stephen Smalley wrote:
> 
> I don't think you want to just set the incore inode security state on
> the client side, as that won't be preserved.

I agree. The fix seems much worse than the problem.

It doesn't seem to be a kernel bug (except insofar that you can argue that 
enabling ACL's at all is a bug), and it also doesn't look like a 
regression.

		Linus

Re: [NFS] [PATCH 3/3] NFS: Ensure we support selinux xattrs

[Trond Myklebust]

On Wed, 2007-01-24 at 12:29 -0800, Linus Torvalds wrote:
> 
> On Wed, 24 Jan 2007, Stephen Smalley wrote:
> > 
> > I don't think you want to just set the incore inode security state on
> > the client side, as that won't be preserved.
> 
> I agree. The fix seems much worse than the problem.
> 
> It doesn't seem to be a kernel bug (except insofar that you can argue that 
> enabling ACL's at all is a bug), and it also doesn't look like a 
> regression.

Fair enough. Please drop this patch then.

Trond