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

From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Djalal Harouni <tixxdz@gmail.com>, Chris Mason <clm@fb.com>,
	Theodore Tso <tytso@mit.edu>,
	Josh Triplett <josh@joshtriplett.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Andy Lutomirski <luto@kernel.org>,
	Seth Forshee <seth.forshee@canonical.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Dongsu Park <dongsu@endocode.com>,
	David Herrmann <dh.herrmann@googlemail.com>,
	Miklos Szeredi <mszeredi@redhat.com>,
	Alban Crequy <alban.crequy@gmail.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Phil Estes <estesp@gmail.com>
Subject: Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount
Date: Fri, 17 Feb 2017 12:50:12 -0800	[thread overview]
Message-ID: <1487364612.4351.34.camel@HansenPartnership.com> (raw)
In-Reply-To: <20170217175118.GF29622@ZenIV.linux.org.uk>

On Fri, 2017-02-17 at 17:51 +0000, Al Viro wrote:
> On Fri, Feb 17, 2017 at 09:24:40AM -0800, James Bottomley wrote:
> 
> > > What happens when somebody comes along and creates the damn thing
> > > on 
> > > the underlying fs?  _Not_ via your code, that is - using the 
> > > underlying fs mounted elsewhere.
> > 
> > Point taken.  This, I think fixes the dcache revalidation issue.
> 
> No, it doesn't.  Consider a local filesystem.  Those do not have any
> ->d_revalidate() - the kernel bloody well knows what happens to
> directories.  If e.g. a previously absent file gets created, it's
> been done by the kernel itself and dentry has been made positive; if
> a previously existing file has been removed, dentry has either become
> negative or, if it had been pinned (e.g. file was opened at the time,
> or your code had been holding a reference to it, etc.) it will be 
> unhashed so that new lookups won't find it, etc.  No need to 
> revalidate anything.
> 
> Now, consider your code.  You've done a lookup in the underlying fs.
> It has, at the time, come negative, so you have your (negative) 
> dentry pointing to that on the underlying fs.  If somebody comes and 
> does e.g. mkdir() via your fs, it will call vfs_mkdir() on the 
> underlying sucker, hopefully turning it positive and associate a new 
> in-core inode with your previously negative dentry.  But what happens 
> if mkdir is done via underlying fs, or via another instance of yours 
> over the same tree?
> Underlying dentry goes positive; yours is still negative.  The 
> underlying fs either doesn't have ->d_revalidate() or, if there is 
> one it says that the underlying dentry is valid, thank you very much, 
> no need to invalidate anything.
> 
> In other words, your patch does nothing for object getting created.

Right at the moment, the upper layer doesn't cache negative dentries,
but that's only a partial solution.  I assume you'd now like me to
cache negative dentries (principle of least surprise) and handle the
underlying negative to positive transition in d_revalidate?

I can do that.

James

---

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index a4a1f98..5b50447 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -118,9 +118,50 @@ static struct dentry *shiftfs_d_real(struct dentry *dentry,
 	return real;
 }
 
+static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
+{
+	struct dentry *real = dentry->d_fsdata;
+
+	if (d_unhashed(real))
+		return 0;
+
+	if (!(real->d_flags & DCACHE_OP_WEAK_REVALIDATE))
+		return 1;
+
+	return real->d_op->d_weak_revalidate(real, flags);
+}
+
+static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
+{
+	struct dentry *real = dentry->d_fsdata;
+	int ret;
+
+	if (d_unhashed(real))
+		return 0;
+
+	/*
+	 * inode state of underlying changed from positive to negative
+	 * or vice versa; force a lookup to update our view
+	 */
+	if (d_is_negative(real) != d_is_negative(dentry))
+		return 0;
+
+	if (!(real->d_flags & DCACHE_OP_REVALIDATE))
+		return 1;
+
+	ret = real->d_op->d_revalidate(real, flags);
+
+	if (ret == 0 && !(flags & LOOKUP_RCU))
+		d_invalidate(real);
+
+	return ret;
+}
+
 static const struct dentry_operations shiftfs_dentry_ops = {
 	.d_release	= shiftfs_d_release,
 	.d_real		= shiftfs_d_real,
+	.d_revalidate	= shiftfs_d_revalidate,
+	.d_weak_revalidate = shiftfs_d_weak_revalidate,
 };
 
 static int shiftfs_readlink(struct dentry *dentry, char __user *data,
@@ -423,7 +464,7 @@ static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
 	dentry->d_fsdata = new;
 
 	if (!new->d_inode)
-		return NULL;
+		goto out;
 
 	newi = shiftfs_new_inode(dentry->d_sb, new->d_inode->i_mode, new);
 	if (!newi) {
@@ -431,9 +472,8 @@ static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	d_splice_alias(newi, dentry);
-
-	return NULL;
+ out:
+	return d_splice_alias(newi, dentry);
 }
 
 static int shiftfs_permission(struct inode *inode, int mask)
