Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1583041 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-17 03:30 +0100
    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-17 18:30 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-17 20:10 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Vivek Goyal <vgoyal@redhat.com> - 2017-02-17 21:30 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-17 22:00 +0100

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

On Sat, Feb 04, 2017 at 11:19:32AM -0800, James Bottomley wrote:

> +static const struct dentry_operations shiftfs_dentry_ops = {
> +	.d_release	= shiftfs_d_release,
> +	.d_real		= shiftfs_d_real,
> +};

In other words, those dentries are *never* revalidated.  Nevermind that
underlying fs might be mounted elsewhere and be actively modified under
you.

> +static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
> +				     unsigned int flags)
> +{
> +	struct dentry *real = dir->i_private, *new;
> +	struct inode *reali = real->d_inode, *newi;
> +	const struct cred *oldcred, *newcred;
> +
> +	inode_lock(reali);
> +	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
> +	new = lookup_one_len(dentry->d_name.name, real, dentry->d_name.len);
> +	shiftfs_old_creds(oldcred, &newcred);
> +	inode_unlock(reali);
> +
> +	if (IS_ERR(new))
> +		return new;
> +
> +	dentry->d_fsdata = new;
> +
> +	if (!new->d_inode)
> +		return NULL;

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.

[toc] | [next] | [standalone]

#1583620

On Fri, 2017-02-17 at 02:29 +0000, Al Viro wrote:
> On Sat, Feb 04, 2017 at 11:19:32AM -0800, James Bottomley wrote:
> 
> > +static const struct dentry_operations shiftfs_dentry_ops = {
> > +	.d_release	= shiftfs_d_release,
> > +	.d_real		= shiftfs_d_real,
> > +};
> 
> In other words, those dentries are *never* revalidated.  Nevermind 
> that underlying fs might be mounted elsewhere and be actively 
> modified under you.
> 
> > +static struct dentry *shiftfs_lookup(struct inode *dir, struct
> > dentry *dentry,
> > +				     unsigned int flags)
> > +{
> > +	struct dentry *real = dir->i_private, *new;
> > +	struct inode *reali = real->d_inode, *newi;
> > +	const struct cred *oldcred, *newcred;
> > +
> > +	inode_lock(reali);
> > +	oldcred = shiftfs_new_creds(&newcred, dentry->d_sb);
> > +	new = lookup_one_len(dentry->d_name.name, real, dentry
> > ->d_name.len);
> > +	shiftfs_old_creds(oldcred, &newcred);
> > +	inode_unlock(reali);
> > +
> > +	if (IS_ERR(new))
> > +		return new;
> > +
> > +	dentry->d_fsdata = new;
> > +
> > +	if (!new->d_inode)
> > +		return NULL;
> 
> 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.

James

---

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index a4a1f98..1e71efe 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -118,9 +118,43 @@ 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;
+
+	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,
@@ -431,9 +465,7 @@ static struct dentry *shiftfs_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	d_splice_alias(newi, dentry);
-
-	return NULL;
+	return d_splice_alias(newi, dentry);
 }
 
 static int shiftfs_permission(struct inode *inode, int mask)

[toc] | [prev] | [next] | [standalone]

#1583666

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.

[toc] | [prev] | [next] | [standalone]

#1583720

On Fri, Feb 17, 2017 at 05:51:18PM +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.

I thought assumption here is that underlying subtree is not changed
outside of shiftfs. IIUC, overlayfs has the same assumption.

Two shiftfs instances writing to same dir will be a problem though.

Vivek

[toc] | [prev] | [next] | [standalone]

#1583740

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)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web