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

Groups > linux.kernel > #1581642 > 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 Vivek Goyal <vgoyal@redhat.com> - 2017-02-15 21:40 +0100
    Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-16 17:00 +0100
      Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Al Viro <viro@ZenIV.linux.org.uk> - 2017-02-17 04:00 +0100
        Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-17 18:40 +0100
          Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Vivek Goyal <vgoyal@redhat.com> - 2017-02-17 21:50 +0100
            Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-19 04:30 +0100
              Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount Vivek Goyal <vgoyal@redhat.com> - 2017-02-20 20:30 +0100
                Re: [RFC 1/1] shiftfs: uid/gid shifting bind mount James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-02-21 01:40 +0100

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

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

[..]
> +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;
> +
> +	newi = shiftfs_new_inode(dentry->d_sb, new->d_inode->i_mode, new);
> +	if (!newi) {
> +		dput(new);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	d_splice_alias(newi, dentry);

Hi James,

Should it be "return d_splice_alias()" so that if we find an alias it is
returned back to caller and passed in dentry can be freed. Though I don't
know in what cases alias can be found. And if alias is found how do we
make sure alias_dentry->d_fsdata is pointing to new (real dentry).

> +
> +	return NULL;
> +}

Vivek

[toc] | [next] | [standalone]

#1582659

On Wed, 2017-02-15 at 15:34 -0500, Vivek Goyal wrote:
> On Sat, Feb 04, 2017 at 11:19:32AM -0800, James Bottomley wrote:
> 
> [..]
> > +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;
> > +
> > +	newi = shiftfs_new_inode(dentry->d_sb, new->d_inode
> > ->i_mode, new);
> > +	if (!newi) {
> > +		dput(new);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	d_splice_alias(newi, dentry);
> 
> Hi James,
> 
> Should it be "return d_splice_alias()" so that if we find an alias it 
> is returned back to caller and passed in dentry can be freed. Though 
> I don't know in what cases alias can be found. And if alias is found 
> how do we make sure alias_dentry->d_fsdata is pointing to new (real
> dentry).

It probably should be for the sake of the pattern.  In our case I don't
think we can have any root aliases because the root dentry is always
pinned in the cache, so cache lookup should always find it.

James

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

#1583046

On Thu, Feb 16, 2017 at 07:56:30AM -0800, James Bottomley wrote:

> > Hi James,
> > 
> > Should it be "return d_splice_alias()" so that if we find an alias it 
> > is returned back to caller and passed in dentry can be freed. Though 
> > I don't know in what cases alias can be found. And if alias is found 
> > how do we make sure alias_dentry->d_fsdata is pointing to new (real
> > dentry).
> 
> It probably should be for the sake of the pattern.  In our case I don't
> think we can have any root aliases because the root dentry is always
> pinned in the cache, so cache lookup should always find it.

What does that have to do with root dentry?  The real reason why that code
works (FVerySVO) is that the damn thing allocates a new inode every time.
Including the hardlinks, BTW.  So d_splice_alias() will always return NULL -
there's no way for any dentries to be pointing to in-core struct inode you've
just allocated.  Short of a use-after-free, that is...

Unless I'm missing something subtle, the whole thing is fucked
in head wrt cache coherency - its dentries are blindly assumed to be
forever valid, no matter what's happening with the underlying filesystem.

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

#1583624

On Fri, 2017-02-17 at 02:55 +0000, Al Viro wrote:
> On Thu, Feb 16, 2017 at 07:56:30AM -0800, James Bottomley wrote:
> 
> > > Hi James,
> > > 
> > > Should it be "return d_splice_alias()" so that if we find an 
> > > alias it is returned back to caller and passed in dentry can be 
> > > freed. Though I don't know in what cases alias can be found. And 
> > > if alias is found how do we make sure alias_dentry->d_fsdata is 
> > > pointing to new (real dentry).
> > 
> > It probably should be for the sake of the pattern.  In our case I 
> > don't think we can have any root aliases because the root dentry is
> > always pinned in the cache, so cache lookup should always find it.
> 
> What does that have to do with root dentry?  The real reason why that 
> code works (FVerySVO) is that the damn thing allocates a new inode 
> every time. Including the hardlinks, BTW.

Yes, this is a known characteristic of stacked filesystems.  Is there
some magic I don't know about that would make it easier to reflect hard
links as aliases?

>   So d_splice_alias() will always return NULL - there's no way for 
> any dentries to be pointing to in-core struct inode you've
> just allocated.  Short of a use-after-free, that is...
> 
> Unless I'm missing something subtle, the whole thing is fucked
> in head wrt cache coherency - its dentries are blindly assumed to be
> forever valid, no matter what's happening with the underlying 
> filesystem.

Hopefully the patch in the previous email fixes this.

James

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

#1583734

On Fri, Feb 17, 2017 at 09:34:07AM -0800, James Bottomley wrote:
> On Fri, 2017-02-17 at 02:55 +0000, Al Viro wrote:
> > On Thu, Feb 16, 2017 at 07:56:30AM -0800, James Bottomley wrote:
> > 
> > > > Hi James,
> > > > 
> > > > Should it be "return d_splice_alias()" so that if we find an 
> > > > alias it is returned back to caller and passed in dentry can be 
> > > > freed. Though I don't know in what cases alias can be found. And 
> > > > if alias is found how do we make sure alias_dentry->d_fsdata is 
> > > > pointing to new (real dentry).
> > > 
> > > It probably should be for the sake of the pattern.  In our case I 
> > > don't think we can have any root aliases because the root dentry is
> > > always pinned in the cache, so cache lookup should always find it.
> > 
> > What does that have to do with root dentry?  The real reason why that 
> > code works (FVerySVO) is that the damn thing allocates a new inode 
> > every time. Including the hardlinks, BTW.
> 
> Yes, this is a known characteristic of stacked filesystems.  Is there
> some magic I don't know about that would make it easier to reflect hard
> links as aliases?

I think overlayfs had the same issue in the beginning and miklos fixed it.

commit 51f7e52dc943468c6929fa0a82d4afac3c8e9636
Author: Miklos Szeredi <mszeredi@redhat.com>
Date:   Fri Jul 29 12:05:24 2016 +0200

    ovl: share inode for hard link

Vivek

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

#1584061

On Fri, 2017-02-17 at 15:35 -0500, Vivek Goyal wrote:
> On Fri, Feb 17, 2017 at 09:34:07AM -0800, James Bottomley wrote:
> > On Fri, 2017-02-17 at 02:55 +0000, Al Viro wrote:
> > > On Thu, Feb 16, 2017 at 07:56:30AM -0800, James Bottomley wrote:
> > > 
> > > > > Hi James,
> > > > > 
> > > > > Should it be "return d_splice_alias()" so that if we find an 
> > > > > alias it is returned back to caller and passed in dentry can 
> > > > > be freed. Though I don't know in what cases alias can be 
> > > > > found. And if alias is found how do we make sure alias_dentry
> > > > > ->d_fsdata is pointing to new (real dentry).
> > > > 
> > > > It probably should be for the sake of the pattern.  In our case 
> > > > I don't think we can have any root aliases because the root
> > > > dentry is always pinned in the cache, so cache lookup should 
> > > > always find it.
> > > 
> > > What does that have to do with root dentry?  The real reason why 
> > > that code works (FVerySVO) is that the damn thing allocates a new
> > > inode every time. Including the hardlinks, BTW.
> > 
> > Yes, this is a known characteristic of stacked filesystems.  Is 
> > there some magic I don't know about that would make it easier to 
> > reflect hard links as aliases?
> 
> I think overlayfs had the same issue in the beginning and miklos
> fixed it.
> 
> commit 51f7e52dc943468c6929fa0a82d4afac3c8e9636
> Author: Miklos Szeredi <mszeredi@redhat.com>
> Date:   Fri Jul 29 12:05:24 2016 +0200
> 
>     ovl: share inode for hard link

That's rather complex, but the principle is simple: use the inode hash
for all upper inodes that may have aliases.  Aliasable means the
underlying inode isn't a directory and has i_nlink > 1, so all I have
to do is perform a lookup through the hash if the underlying is
aliasable, invalidate the dentry in d_revalidate if the aliasing
conditions to the underlying change and manually handle hard links and
it should all work.

Like this?

James

---

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index 5b50447..c659812 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -134,6 +134,7 @@ static int shiftfs_d_weak_revalidate(struct dentry *dentry, unsigned int flags)
 static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct dentry *real = dentry->d_fsdata;
+	struct inode *reali = d_inode(real), *inode = d_inode(dentry);
 	int ret;
 
 	if (d_unhashed(real))
@@ -146,6 +147,15 @@ static int shiftfs_d_revalidate(struct dentry *dentry, unsigned int flags)
 	if (d_is_negative(real) != d_is_negative(dentry))
 		return 0;
 
+	/*
+	 * non dir link count is > 1 and our inode is currently not in
+	 * the inode hash => need to drop and reget our dentry to make
+	 * sure we're aliasing it correctly.
+	 */
+	if (reali &&!S_ISDIR(reali->i_mode) && reali->i_nlink > 1 &&
+	    (!inode || inode_unhashed(inode)))
+		return 0;
+
 	if (!(real->d_flags & DCACHE_OP_REVALIDATE))
 		return 1;
 
@@ -285,7 +295,8 @@ static int shiftfs_make_object(struct inode *dir, struct dentry *dentry,
 			       umode_t mode, const char *symlink,
 			       struct dentry *hardlink, bool excl)
 {
-	struct dentry *real = dir->i_private, *new = dentry->d_fsdata;
+	struct dentry *real = dir->i_private, *new = dentry->d_fsdata,
+		*realhardlink = NULL;
 	struct inode *reali = real->d_inode, *newi;
 	const struct inode_operations *iop = reali->i_op;
 	int err;
@@ -293,6 +304,7 @@ static int shiftfs_make_object(struct inode *dir, struct dentry *dentry,
 	bool op_ok = false;
 
 	if (hardlink) {
+		realhardlink = hardlink->d_fsdata;
 		op_ok = iop->link;
 	} else {
 		switch (mode & S_IFMT) {
@@ -310,7 +322,7 @@ static int shiftfs_make_object(struct inode *dir, struct dentry *dentry,
 		return -EINVAL;
 
 
-	newi = shiftfs_new_inode(dentry->d_sb, mode, NULL);
+	newi = shiftfs_new_inode(dentry->d_sb, mode, realhardlink);
 	if (!newi)
 		return -ENOMEM;
 
@@ -320,8 +332,6 @@ static int shiftfs_make_object(struct inode *dir, struct dentry *dentry,
 
 	err = -EINVAL;		/* shut gcc up about uninit var */
 	if (hardlink) {
-		struct dentry *realhardlink = hardlink->d_fsdata;
-
 		err = vfs_link(realhardlink, reali, new, NULL);
 	} else {
 		switch (mode & S_IFMT) {
@@ -341,7 +351,16 @@ static int shiftfs_make_object(struct inode *dir, struct dentry *dentry,
 	if (err)
 		goto out_dput;
 
-	shiftfs_fill_inode(newi, new);
+	if (!hardlink)
+		shiftfs_fill_inode(newi, new);
+	else if (inode_unhashed(newi) && !S_ISDIR(newi->i_mode))
+		/*
+		 * although dentry and hardlink now each point to
+		 * newi, the link count was 1 when they were created,
+		 * so insert into the inode cache now that the link
+		 * count has gone above one.
+		 */
+		__insert_inode_hash(newi, (unsigned long)d_inode(new));
 
 	d_instantiate(dentry, newi);
 
@@ -569,12 +588,55 @@ static const struct inode_operations shiftfs_inode_ops = {
 	.listxattr	= shiftfs_listxattr,
 };
 
+static int shiftfs_test(struct inode *inode, void *data)
+{
+	struct dentry *d1 = inode->i_private, *d2 = data;
+	struct inode *i1 = d_inode(d1), *i2 = d_inode(d2);
+
+	return i1 && i1 == i2;
+}
+
+static int shiftfs_set(struct inode *inode, void *data)
+{
+	struct dentry *dentry = data;
+
+	shiftfs_fill_inode(inode, dentry);
+
+	return 0;
+}
+
 static struct inode *shiftfs_new_inode(struct super_block *sb, umode_t mode,
 				       struct dentry *dentry)
 {
 	struct inode *inode;
+	struct inode *reali = dentry ? d_inode(dentry): NULL;
+	bool use_inode_hash = false;
+
+	/*
+	 * Here we hash the inode only if the underlying link count is
+	 * greater than one and it's not a directory (meaning the hash
+	 * contains all items that might be aliases).  We keep this
+	 * accurate by checking the underlying link count on
+	 * revalidation and forcing a new lookup if the underlying
+	 * link count is raised.
+	 *
+	 * Note: if the link count drops again, we don't remove the
+	 * inode from the hash, so the hash contains all inodes that
+	 * may be aliases plus a few others.
+	 */
+	if (reali)
+		use_inode_hash = ACCESS_ONCE(reali->i_nlink) > 1 &&
+			!S_ISDIR(reali->i_mode);
+
+	if (use_inode_hash) {
+		inode = iget5_locked(sb, (unsigned long)reali, shiftfs_test,
+				     shiftfs_set, dentry);
+		if (inode && !(inode->i_state & I_NEW))
+			return inode;
+	} else {
+		inode = new_inode(sb);
+	}
 
-	inode = new_inode(sb);
 	if (!inode)
 		return NULL;
 
@@ -586,7 +648,10 @@ static struct inode *shiftfs_new_inode(struct super_block *sb, umode_t mode,
 
 	inode->i_op = &shiftfs_inode_ops;
 
-	shiftfs_fill_inode(inode, dentry);
+	if (use_inode_hash)
+		unlock_new_inode(inode);
+	else
+		shiftfs_fill_inode(inode, dentry);
 
 	return inode;
 }

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

#1584855

On Sat, Feb 18, 2017 at 07:24:38PM -0800, James Bottomley wrote:

[..]
> > > Yes, this is a known characteristic of stacked filesystems.  Is 
> > > there some magic I don't know about that would make it easier to 
> > > reflect hard links as aliases?
> > 
> > I think overlayfs had the same issue in the beginning and miklos
> > fixed it.
> > 
> > commit 51f7e52dc943468c6929fa0a82d4afac3c8e9636
> > Author: Miklos Szeredi <mszeredi@redhat.com>
> > Date:   Fri Jul 29 12:05:24 2016 +0200
> > 
> >     ovl: share inode for hard link
> 
> That's rather complex, but the principle is simple: use the inode hash
> for all upper inodes that may have aliases.  Aliasable means the
> underlying inode isn't a directory and has i_nlink > 1, so all I have
> to do is perform a lookup through the hash if the underlying is
> aliasable, invalidate the dentry in d_revalidate if the aliasing
> conditions to the underlying change and manually handle hard links and
> it should all work.
> 
> Like this?

Sounds reasonable to me. I did basic testing and this seems to work for me.

In general, I am having random crashes. I just get following on serial
console

------[Cut Here]----------

And nothing after that.

Still trying to narrow down.

Vivek

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

#1584978

On Mon, 2017-02-20 at 14:26 -0500, Vivek Goyal wrote:
> On Sat, Feb 18, 2017 at 07:24:38PM -0800, James Bottomley wrote:
> 
> [..]
> > > > Yes, this is a known characteristic of stacked filesystems.  Is
> > > > there some magic I don't know about that would make it easier
> > > > to 
> > > > reflect hard links as aliases?
> > > 
> > > I think overlayfs had the same issue in the beginning and miklos
> > > fixed it.
> > > 
> > > commit 51f7e52dc943468c6929fa0a82d4afac3c8e9636
> > > Author: Miklos Szeredi <mszeredi@redhat.com>
> > > Date:   Fri Jul 29 12:05:24 2016 +0200
> > > 
> > >     ovl: share inode for hard link
> > 
> > That's rather complex, but the principle is simple: use the inode
> > hash
> > for all upper inodes that may have aliases.  Aliasable means the
> > underlying inode isn't a directory and has i_nlink > 1, so all I
> > have
> > to do is perform a lookup through the hash if the underlying is
> > aliasable, invalidate the dentry in d_revalidate if the aliasing
> > conditions to the underlying change and manually handle hard links
> > and
> > it should all work.
> > 
> > Like this?
> 
> Sounds reasonable to me. I did basic testing and this seems to work
> for me.
> 
> In general, I am having random crashes. I just get following on 
> serial console
> 
> ------[Cut Here]----------
> 
> And nothing after that.

That's indicative of some hard lockup.  I don't see this, but I'm also
using a second laptop for testing, which is suboptimal.  I'm going to
try moving to xfstests inside a VM tomorrow (that's what long aeroplane
flights are for).

> Still trying to narrow down.

Thanks.  There've been a lot of patches flying around, so I'll do a
collected repost under a v2 header to make sure we're all in sync.

James

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web