Skip to contentSkip to conversation

add vfs_* helpers for xattr operations

PATCH In review Christoph Hellwig 2 participants 2 messages Nov 1, 2005

Subsystems (2)
VFS NFS server
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/8 add vfs_* helpers for xattr operations

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 <[email protected]>
fs/nfsd/vfs.c +13 -35
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
@@ -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;
 		}
fs/xattr.c +95 -53

178 lines not on this page · raw message

include/linux/xattr.h +4 -0
Index: linux-2.6/include/linux/xattr.h
===================================================================
@@ -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);
#1 lore raw
Quoted 9 lines · 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?

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.

Signature · 2 lines
Stephen Smalley
National Security Agency