[PATCH 1/8] add vfs_* helpers for xattr operations

[PATCH 1/8] add vfs_* helpers for xattr operations

[Christoph Hellwig]

Add vfs_getxattr, vfs_setxattr and vfs_removexattr helpers for common
checks around invocation of the xattr methods.  NFSD already was missing
some of the checks and there will be more soon.

James, I haven't touched selinux yet because it's doing various odd
things and I'm not sure how it would interact with the security
attribute fallbacks you added.  Could you investigate whether it could
use vfs_getxattr or if not add a __vfs_getxattr helper to share the bits
it is fine with?


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
--- linux-2.6.orig/fs/nfsd/vfs.c	2005-10-31 14:34:22.000000000 +0100
+++ linux-2.6/fs/nfsd/vfs.c	2005-10-31 14:34:33.000000000 +0100
@@ -367,7 +367,6 @@
 	size_t buflen;
 	char *buf = NULL;
 	int error = 0;
-	struct inode *inode = dentry->d_inode;
 
 	buflen = posix_acl_xattr_size(pacl->a_count);
 	buf = kmalloc(buflen, GFP_KERNEL);
@@ -381,15 +380,7 @@
 		goto out;
 	}
 
-	error = -EOPNOTSUPP;
-	if (inode->i_op && inode->i_op->setxattr) {
-		down(&inode->i_sem);
-		security_inode_setxattr(dentry, key, buf, len, 0);
-		error = inode->i_op->setxattr(dentry, key, buf, len, 0);
-		if (!error)
-			security_inode_post_setxattr(dentry, key, buf, len, 0);
-		up(&inode->i_sem);
-	}
+	error = vfs_setxattr(dentry, key, buf, len, 0);
 out:
 	kfree(buf);
 	return error;
@@ -448,44 +439,32 @@
 static struct posix_acl *
 _get_posix_acl(struct dentry *dentry, char *key)
 {
-	struct inode *inode = dentry->d_inode;
 	char *buf = NULL;
 	int buflen, error = 0;
 	struct posix_acl *pacl = NULL;
 
-	error = -EOPNOTSUPP;
-	if (inode->i_op == NULL)
-		goto out_err;
-	if (inode->i_op->getxattr == NULL)
-		goto out_err;
-
-	error = security_inode_getxattr(dentry, key);
-	if (error)
-		goto out_err;
-
-	buflen = inode->i_op->getxattr(dentry, key, NULL, 0);
+	buflen = vfs_getxattr(dentry, key, NULL, 0);
 	if (buflen <= 0) {
-		error = buflen < 0 ? buflen : -ENODATA;
-		goto out_err;
+		pacl = ERR_PTR(buflen < 0 ? buflen : -ENODATA);
+		goto out;
 	}
 
 	buf = kmalloc(buflen, GFP_KERNEL);
 	if (buf == NULL) {
-		error = -ENOMEM;
-		goto out_err;
+		pacl = ERR_PTR(-ENOMEM);
+		goto out;
 	}
 
-	error = inode->i_op->getxattr(dentry, key, buf, buflen);
-	if (error < 0)
-		goto out_err;
+	error = vfs_getxattr(dentry, key, buf, buflen);
+	if (error < 0) {
+		pacl = ERR_PTR(error);
+		goto out;
+	}
 
 	pacl = posix_acl_from_xattr(buf, buflen);
  out:
 	kfree(buf);
 	return pacl;
- out_err:
-	pacl = ERR_PTR(error);
-	goto out;
 }
 
 int
@@ -1943,13 +1922,12 @@
 	if (!fhp->fh_locked)
 		fh_lock(fhp);  /* unlocking is done automatically */
 	if (size)
-		error = inode->i_op->setxattr(fhp->fh_dentry, name,
-					      value, size, 0);
+		error = vfs_setxattr(fhp->fh_dentry, name, value, size, 0);
 	else {
 		if (!S_ISDIR(inode->i_mode) && type == ACL_TYPE_DEFAULT)
 			error = 0;
 		else {
-			error = inode->i_op->removexattr(fhp->fh_dentry, name);
+			error = vfs_removexattr(fhp->fh_dentry, name);
 			if (error == -ENODATA)
 				error = 0;
 		}
Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2005-10-31 14:34:22.000000000 +0100
+++ linux-2.6/fs/xattr.c	2005-10-31 14:42:26.000000000 +0100
@@ -19,6 +19,96 @@
 #include <linux/fsnotify.h>
 #include <asm/uaccess.h>
 
+
+int
+vfs_setxattr(struct dentry *dentry, char *name, void *value,
+		size_t size, int flags)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	down(&inode->i_sem);
+	error = security_inode_setxattr(dentry, name, value, size, flags);
+	if (error)
+		goto out;
+	error = -EOPNOTSUPP;
+	if (inode->i_op->setxattr) {
+		error = inode->i_op->setxattr(dentry, name, value, size, flags);
+		if (!error) {
+			fsnotify_xattr(dentry);
+			security_inode_post_setxattr(dentry, name, value,
+						     size, flags);
+		}
+	} else if (!strncmp(name, XATTR_SECURITY_PREFIX,
+				sizeof XATTR_SECURITY_PREFIX - 1)) {
+		const char *suffix = name + sizeof XATTR_SECURITY_PREFIX - 1;
+		error = security_inode_setsecurity(inode, suffix, value,
+						   size, flags);
+		if (!error)
+			fsnotify_xattr(dentry);
+	}
+out:
+	up(&inode->i_sem);
+	return error;
+}
+EXPORT_SYMBOL_GPL(vfs_setxattr);
+
+ssize_t
+vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = security_inode_getxattr(dentry, name);
+	if (error)
+		return error;
+
+	if (inode->i_op->getxattr)
+		error = inode->i_op->getxattr(dentry, name, value, size);
+	else
+		error = -EOPNOTSUPP;
+
+	if (!strncmp(name, XATTR_SECURITY_PREFIX,
+				sizeof XATTR_SECURITY_PREFIX - 1)) {
+		const char *suffix = name + sizeof XATTR_SECURITY_PREFIX - 1;
+		int ret = security_inode_getsecurity(inode, suffix, value,
+						     size, error);
+		/*
+		 * Only overwrite the return value if a security module
+		 * is actually active.
+		 */
+		if (ret != -EOPNOTSUPP)
+			error = ret;
+	}
+
+	return error;
+}
+EXPORT_SYMBOL_GPL(vfs_getxattr);
+
+int
+vfs_removexattr(struct dentry *dentry, char *name)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	if (!inode->i_op->removexattr)
+		return -EOPNOTSUPP;
+
+	error = security_inode_removexattr(dentry, name);
+	if (error)
+		return error;
+
+	down(&inode->i_sem);
+	error = inode->i_op->removexattr(dentry, name);
+	up(&inode->i_sem);
+
+	if (!error)
+		fsnotify_xattr(dentry);
+	return error;
+}
+EXPORT_SYMBOL_GPL(vfs_removexattr);
+
+
 /*
  * Extended attribute SET operations
  */
@@ -51,29 +141,8 @@
 		}
 	}
 
-	down(&d->d_inode->i_sem);
-	error = security_inode_setxattr(d, kname, kvalue, size, flags);
-	if (error)
-		goto out;
-	error = -EOPNOTSUPP;
-	if (d->d_inode->i_op && d->d_inode->i_op->setxattr) {
-		error = d->d_inode->i_op->setxattr(d, kname, kvalue,
-						   size, flags);
-		if (!error) {
-			fsnotify_xattr(d);
-			security_inode_post_setxattr(d, kname, kvalue,
-						     size, flags);
-		}
-	} else if (!strncmp(kname, XATTR_SECURITY_PREFIX,
-			    sizeof XATTR_SECURITY_PREFIX - 1)) {
-		const char *suffix = kname + sizeof XATTR_SECURITY_PREFIX - 1;
-		error = security_inode_setsecurity(d->d_inode, suffix, kvalue,
-						   size, flags);
-		if (!error)
-			fsnotify_xattr(d);
-	}
-out:
-	up(&d->d_inode->i_sem);
+	error = vfs_setxattr(d, kname, kvalue, size, flags);
+
 	if (kvalue)
 		kfree(kvalue);
 	return error;
@@ -148,22 +217,7 @@
 			return -ENOMEM;
 	}
 
-	error = security_inode_getxattr(d, kname);
-	if (error)
-		goto out;
-	error = -EOPNOTSUPP;
-	if (d->d_inode->i_op && d->d_inode->i_op->getxattr)
-		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
-
-	if (!strncmp(kname, XATTR_SECURITY_PREFIX,
-		     sizeof XATTR_SECURITY_PREFIX - 1)) {
-		const char *suffix = kname + sizeof XATTR_SECURITY_PREFIX - 1;
-		int rv = security_inode_getsecurity(d->d_inode, suffix, kvalue,
-						    size, error);
-		/* Security module active: overwrite error value */
-		if (rv != -EOPNOTSUPP)
-			error = rv;
-	}
+	error = vfs_getxattr(d, kname, kvalue, size);
 	if (error > 0) {
 		if (size && copy_to_user(value, kvalue, error))
 			error = -EFAULT;
@@ -172,7 +226,7 @@
 		   than XATTR_SIZE_MAX bytes. Not possible. */
 		error = -E2BIG;
 	}
-out:
+
 	if (kvalue)
 		kfree(kvalue);
 	return error;
@@ -321,19 +375,7 @@
 	if (error < 0)
 		return error;
 
-	error = -EOPNOTSUPP;
-	if (d->d_inode->i_op && d->d_inode->i_op->removexattr) {
-		error = security_inode_removexattr(d, kname);
-		if (error)
-			goto out;
-		down(&d->d_inode->i_sem);
-		error = d->d_inode->i_op->removexattr(d, kname);
-		up(&d->d_inode->i_sem);
-		if (!error)
-			fsnotify_xattr(d);
-	}
-out:
-	return error;
+	return vfs_removexattr(d, kname);
 }
 
 asmlinkage long
Index: linux-2.6/include/linux/xattr.h
===================================================================
--- linux-2.6.orig/include/linux/xattr.h	2005-10-31 14:34:22.000000000 +0100
+++ linux-2.6/include/linux/xattr.h	2005-10-31 14:42:12.000000000 +0100
@@ -25,6 +25,10 @@
 		   size_t size, int flags);
 };
 
+ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
+int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
+int vfs_removexattr(struct dentry *, char *);
+
 ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
 ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size);
 int generic_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags);

Re: [PATCH 1/8] add vfs_* helpers for xattr operations

[Stephen Smalley]

On Tue, 2005-11-01 at 03:30 +0100, Christoph Hellwig wrote:
> Add vfs_getxattr, vfs_setxattr and vfs_removexattr helpers for common
> checks around invocation of the xattr methods.  NFSD already was missing
> some of the checks and there will be more soon.
> 
> James, I haven't touched selinux yet because it's doing various odd
> things and I'm not sure how it would interact with the security
> attribute fallbacks you added.  Could you investigate whether it could
> use vfs_getxattr or if not add a __vfs_getxattr helper to share the bits
> it is fine with?

Hi,

On the latter point:  I don't believe that the SELinux "module" can
re-use any of the vfs_getxattr logic for its internal calls to
i_op->getxattr, because SELinux is using i_op->getxattr to fetch the
attribute when it is setting up the incore inode security structure.  We
do not want the permission checking (call to security_inode_getxattr) of
vfs_getxattr on such internal setup, and we do not want the fallback
behavior (because the fallback proceeds to ask SELinux for an attribute
based on the incore inode security structure, which we are in the
process of setting up here).  SELinux wants to interrogate the specific
fs for the attribute at this point, and further wants to handle failures
itself (e.g. detecting a lack of necessary xattr support in the fs at
mount time, mapping files that lack an xattr to a default label at inode
security structure setup time).  Hence, calling i_op->getxattr seems
correct for internal use by the SELinux module.  That isn't to say that
the helper isn't a good idea for other users of i_op->getxattr; I just
don't think it is a good fit for SELinux. 

-- 
Stephen Smalley
National Security Agency