Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Wed, 2016-05-18 at 21:28 -0500, Serge E. Hallyn wrote:
> Hey James,
> 
> yeah that's a lot better.  I do still get some syslog messages,
> but i was trivially able to bind a shiftfs into a container and
> use it the way I'd want.
> 
> [  209.452274] ------------[ cut here ]------------
> [  209.452296] WARNING: CPU: 0 PID: 3072 at fs/ext4/inode.c:3977 
> ext4_truncate+0x3f5/0x5b0

Heh, I really need to test with ext4; it seems much more careful.  XFS
doesn't warn on any of this.  These are both inode locking problems
with setattr.  It also looks like I'd have the same problem with
setxattr and removexattr.  Does this additional patch allow you to
operate without any warnings?

There's also something else you'll be running into soon: the xattr
calls aren't uid shifted.  I was a bit worried about how to do this
without leaking root attribute setting capability, but I'll think a bit
more carefully about how to do it.

Thanks,

James

---

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index d352377..29f343f 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -240,14 +240,17 @@ static int shiftfs_setxattr(struct dentry *dentry, const char *name,
 			    const void *value, size_t size, int flags)
 {
 	struct dentry *real = dentry->d_fsdata;
-	const struct inode_operations *iop = real->d_inode->i_op;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
 	int err = -EOPNOTSUPP;
 
 	if (iop->setxattr) {
 		const struct cred *oldcred, *newcred;
 
 		oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
+		inode_lock(reali);
 		err = iop->setxattr(real, name, value, size, flags);
+		inode_unlock(reali);
 		shiftfs_old_creds(oldcred, &newcred);
 	}
 
@@ -287,12 +290,17 @@ static ssize_t shiftfs_listxattr(struct dentry *dentry, char *list,
 static int shiftfs_removexattr(struct dentry *dentry, const char *name)
 {
 	struct dentry *real = dentry->d_fsdata;
-	const struct inode_operations *iop = real->d_inode->i_op;
+	struct inode *reali = real->d_inode;
+	const struct inode_operations *iop = reali->i_op;
+	int err = -EINVAL;
 
-	if (iop->removexattr)
-		return iop->removexattr(real, name);
+	if (iop->removexattr) {
+		inode_lock(reali);
+		err = iop->removexattr(real, name);
+		inode_unlock(reali);
+	}
 
-	return -EINVAL;
+	return err;
 }
 
 static void shiftfs_fill_inode(struct inode *inode, struct dentry *dentry)
@@ -548,11 +556,13 @@ static int shiftfs_setattr(struct dentry *dentry, struct iattr *attr)
 	newattr.ia_uid = KUIDT_INIT(map_id_up(&ssi->uid_map, __kuid_val(attr->ia_uid)));
 	newattr.ia_gid = KGIDT_INIT(map_id_up(&ssi->gid_map, __kgid_val(attr->ia_gid)));
 
+	inode_lock(reali);
 	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
 	if (iop->setattr)
 		err = iop->setattr(real, &newattr);
 	else
 		err = simple_setattr(real, &newattr);
+	inode_unlock(reali);
 	shiftfs_old_creds(oldcred, &newcred);
 
 	return err;