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

Groups > linux.kernel > #1359643 > unrolled thread

[PATCH 1/4] vfs: add file_dentry()

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi <miklos@szeredi.hu> - 2016-03-17 10:10 +0100
    [PATCH 3/4] ext4: use dget_parent() in ext4_file_open() Miklos Szeredi <miklos@szeredi.hu> - 2016-03-17 10:10 +0100
    [PATCH 4/4] ext4: use file_dentry() Miklos Szeredi <miklos@szeredi.hu> - 2016-03-17 10:10 +0100
    Re: [PATCH 1/4] vfs: add file_dentry() Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-17 10:20 +0100
      Re: [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi <miklos@szeredi.hu> - 2016-03-17 10:40 +0100
        Re: [PATCH 1/4] vfs: add file_dentry() Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-17 11:20 +0100
        Re: [PATCH 1/4] vfs: add file_dentry() Sedat Dilek <sedat.dilek@gmail.com> - 2016-03-17 11:20 +0100
          Re: [PATCH 1/4] vfs: add file_dentry() Theodore Ts'o <tytso@mit.edu> - 2016-03-17 15:20 +0100
    Re: [PATCH 1/4] vfs: add file_dentry() Theodore Ts'o <tytso@mit.edu> - 2016-03-21 06:10 +0100
      Re: [PATCH 1/4] vfs: add file_dentry() Al Viro <viro@ZenIV.linux.org.uk> - 2016-03-21 06:30 +0100
      Re: [PATCH 1/4] vfs: add file_dentry() Daniel Axtens <dja@axtens.net> - 2016-03-22 07:30 +0100
    Re: [PATCH 1/4] vfs: add file_dentry() Al Viro <viro@ZenIV.linux.org.uk> - 2016-03-21 06:30 +0100
      Re: [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi <miklos@szeredi.hu> - 2016-03-21 09:20 +0100

#1359643 — [PATCH 1/4] vfs: add file_dentry()

From: Miklos Szeredi <mszeredi@redhat.com>

This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
f_path always point to the overlay and f_inode to the underlay").

Regular files opened on overlayfs will result in the file being opened on
the underlying filesystem, while f_path points to the overlayfs
mount/dentry.

This confuses filesystems which get the dentry from struct file and assume
it's theirs.

Add a new helper, file_dentry() [*], to get the filesystem's own dentry
from the file.  This simply compares file_inode(file->f_path.dentry) to
file_inode(file) and if they are equal returns file->f_path.dentry (this is
the common, non-overlayfs case).

In the uncommon case (regular file on overlayfs) it will call into
overlayfs's ->d_native_dentry() to get the underlying dentry matching
file_inode(file).

[*] If possible, it's better simply to use file_inode() instead.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Tested-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: <stable@vger.kernel.org> # v4.2
Cc: David Howells <dhowells@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Daniel Axtens <dja@axtens.net>
---
 fs/open.c              | 11 +++++++++++
 fs/overlayfs/super.c   | 16 ++++++++++++++++
 include/linux/dcache.h |  1 +
 include/linux/fs.h     |  2 ++
 4 files changed, 30 insertions(+)

diff --git a/fs/open.c b/fs/open.c
index 55bdc75e2172..6326c11eda78 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -831,6 +831,17 @@ char *file_path(struct file *filp, char *buf, int buflen)
 }
 EXPORT_SYMBOL(file_path);
 
+struct dentry *file_dentry(const struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+
+	if (likely(d_inode(dentry) == file_inode(file)))
+		return dentry;
+	else
+		return dentry->d_op->d_native_dentry(dentry, file_inode(file));
+}
+EXPORT_SYMBOL(file_dentry);
+
 /**
  * vfs_open - open the file at the given path
  * @path: path to open
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 619ad4b016d2..5142aa2034c4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
 	return ret;
 }
 
+static struct dentry *ovl_d_native_dentry(struct dentry *dentry,
+					 struct inode *inode)
+{
+	struct ovl_entry *oe = dentry->d_fsdata;
+	struct dentry *realentry = ovl_upperdentry_dereference(oe);
+
+	if (realentry && inode == d_inode(realentry))
+		return realentry;
+	realentry = __ovl_dentry_lower(oe);
+	if (realentry && inode == d_inode(realentry))
+		return realentry;
+	BUG();
+}
+
 static const struct dentry_operations ovl_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_select_inode = ovl_d_select_inode,
+	.d_native_dentry = ovl_d_native_dentry,
 };
 
 static const struct dentry_operations ovl_reval_dentry_operations = {
 	.d_release = ovl_dentry_release,
 	.d_select_inode = ovl_d_select_inode,
+	.d_native_dentry = ovl_d_native_dentry,
 	.d_revalidate = ovl_dentry_revalidate,
 	.d_weak_revalidate = ovl_dentry_weak_revalidate,
 };
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index c4b5f4b3f8f8..99ecb6de636c 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -161,6 +161,7 @@ struct dentry_operations {
 	struct vfsmount *(*d_automount)(struct path *);
 	int (*d_manage)(struct dentry *, bool);
 	struct inode *(*d_select_inode)(struct dentry *, unsigned);
+	struct dentry *(*d_native_dentry)(struct dentry *, struct inode *);
 } ____cacheline_aligned;
 
 /*
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ae681002100a..1091d9f43271 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(const struct file *f)
 	return f->f_inode;
 }
 
+extern struct dentry *file_dentry(const struct file *file);
+
 static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
 {
 	return locks_lock_inode_wait(file_inode(filp), fl);
-- 
2.1.4

[toc] | [next] | [standalone]

#1359646 — [PATCH 3/4] ext4: use dget_parent() in ext4_file_open()

From: Miklos Szeredi <mszeredi@redhat.com>

In f_op->open() lock on parent is not held, so there's no guarantee that
parent dentry won't go away at any time.

Even after this patch there's no guarantee that 'dir' will stay the parent
of 'inode', but at least it won't be freed while being used.

Fixes: ff978b09f973 ("ext4 crypto: move context consistency check to ext4_file_open()")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: <stable@vger.kernel.org> # v4.5
---
 fs/ext4/file.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 4cd318f31cbe..feb9ffc6f20d 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -335,7 +335,7 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 	struct super_block *sb = inode->i_sb;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	struct vfsmount *mnt = filp->f_path.mnt;
-	struct inode *dir = filp->f_path.dentry->d_parent->d_inode;
+	struct dentry *dir;
 	struct path path;
 	char buf[64], *cp;
 	int ret;
@@ -379,14 +379,18 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 		if (ext4_encryption_info(inode) == NULL)
 			return -ENOKEY;
 	}
-	if (ext4_encrypted_inode(dir) &&
-	    !ext4_is_child_context_consistent_with_parent(dir, inode)) {
+
+	dir = dget_parent(filp->f_path.dentry);
+	if (ext4_encrypted_inode(d_inode(dir)) &&
+	    !ext4_is_child_context_consistent_with_parent(d_inode(dir), inode)) {
 		ext4_warning(inode->i_sb,
 			     "Inconsistent encryption contexts: %lu/%lu\n",
-			     (unsigned long) dir->i_ino,
+			     (unsigned long) d_inode(dir)->i_ino,
 			     (unsigned long) inode->i_ino);
+		dput(dir);
 		return -EPERM;
 	}
+	dput(dir);
 	/*
 	 * Set up the jbd2_inode if we are opening the inode for
 	 * writing and the journal is present
-- 
2.1.4

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

#1359648 — [PATCH 4/4] ext4: use file_dentry()

From: Miklos Szeredi <mszeredi@redhat.com>

EXT4 may be used as lower layer of overlayfs and accessing f_path.dentry
can lead to a crash.

Fix by replacing direct access of file->f_path.dentry with the
file_dentry() accessor, which will always return a native object.

Reported-by: Daniel Axtens <dja@axtens.net>
Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay")
Fixes: ff978b09f973 ("ext4 crypto: move context consistency check to ext4_file_open()")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: David Howells <dhowells@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: <stable@vger.kernel.org> # v4.5
---
 fs/ext4/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index feb9ffc6f20d..38847f38b34a 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -380,7 +380,7 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 			return -ENOKEY;
 	}
 
-	dir = dget_parent(filp->f_path.dentry);
+	dir = dget_parent(file_dentry(filp));
 	if (ext4_encrypted_inode(d_inode(dir)) &&
 	    !ext4_is_child_context_consistent_with_parent(d_inode(dir), inode)) {
 		ext4_warning(inode->i_sb,
-- 
2.1.4

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

#1359650

On Thu, Mar 17, 2016 at 10:02 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@redhat.com>
>
> This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
> f_path always point to the overlay and f_inode to the underlay").
>

Can you put that series in your vfs.git tree?
Easier for getting and testing.

Thanks.

- Sedat -

> Regular files opened on overlayfs will result in the file being opened on
> the underlying filesystem, while f_path points to the overlayfs
> mount/dentry.
>
> This confuses filesystems which get the dentry from struct file and assume
> it's theirs.
>
> Add a new helper, file_dentry() [*], to get the filesystem's own dentry
> from the file.  This simply compares file_inode(file->f_path.dentry) to
> file_inode(file) and if they are equal returns file->f_path.dentry (this is
> the common, non-overlayfs case).
>
> In the uncommon case (regular file on overlayfs) it will call into
> overlayfs's ->d_native_dentry() to get the underlying dentry matching
> file_inode(file).
>
> [*] If possible, it's better simply to use file_inode() instead.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Tested-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Cc: <stable@vger.kernel.org> # v4.2
> Cc: David Howells <dhowells@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Daniel Axtens <dja@axtens.net>
> ---
>  fs/open.c              | 11 +++++++++++
>  fs/overlayfs/super.c   | 16 ++++++++++++++++
>  include/linux/dcache.h |  1 +
>  include/linux/fs.h     |  2 ++
>  4 files changed, 30 insertions(+)
>
> diff --git a/fs/open.c b/fs/open.c
> index 55bdc75e2172..6326c11eda78 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -831,6 +831,17 @@ char *file_path(struct file *filp, char *buf, int buflen)
>  }
>  EXPORT_SYMBOL(file_path);
>
> +struct dentry *file_dentry(const struct file *file)
> +{
> +       struct dentry *dentry = file->f_path.dentry;
> +
> +       if (likely(d_inode(dentry) == file_inode(file)))
> +               return dentry;
> +       else
> +               return dentry->d_op->d_native_dentry(dentry, file_inode(file));
> +}
> +EXPORT_SYMBOL(file_dentry);
> +
>  /**
>   * vfs_open - open the file at the given path
>   * @path: path to open
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 619ad4b016d2..5142aa2034c4 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
>         return ret;
>  }
>
> +static struct dentry *ovl_d_native_dentry(struct dentry *dentry,
> +                                        struct inode *inode)
> +{
> +       struct ovl_entry *oe = dentry->d_fsdata;
> +       struct dentry *realentry = ovl_upperdentry_dereference(oe);
> +
> +       if (realentry && inode == d_inode(realentry))
> +               return realentry;
> +       realentry = __ovl_dentry_lower(oe);
> +       if (realentry && inode == d_inode(realentry))
> +               return realentry;
> +       BUG();
> +}
> +
>  static const struct dentry_operations ovl_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_select_inode = ovl_d_select_inode,
> +       .d_native_dentry = ovl_d_native_dentry,
>  };
>
>  static const struct dentry_operations ovl_reval_dentry_operations = {
>         .d_release = ovl_dentry_release,
>         .d_select_inode = ovl_d_select_inode,
> +       .d_native_dentry = ovl_d_native_dentry,
>         .d_revalidate = ovl_dentry_revalidate,
>         .d_weak_revalidate = ovl_dentry_weak_revalidate,
>  };
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index c4b5f4b3f8f8..99ecb6de636c 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -161,6 +161,7 @@ struct dentry_operations {
>         struct vfsmount *(*d_automount)(struct path *);
>         int (*d_manage)(struct dentry *, bool);
>         struct inode *(*d_select_inode)(struct dentry *, unsigned);
> +       struct dentry *(*d_native_dentry)(struct dentry *, struct inode *);
>  } ____cacheline_aligned;
>
>  /*
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ae681002100a..1091d9f43271 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(const struct file *f)
>         return f->f_inode;
>  }
>
> +extern struct dentry *file_dentry(const struct file *file);
> +
>  static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  {
>         return locks_lock_inode_wait(file_inode(filp), fl);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1359669

On Thu, Mar 17, 2016 at 10:09 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 10:02 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> From: Miklos Szeredi <mszeredi@redhat.com>
>>
>> This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
>> f_path always point to the overlay and f_inode to the underlay").
>>
>
> Can you put that series in your vfs.git tree?
> Easier for getting and testing.

Ok, pushed to:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git file_dentry

Thanks,
Miklos

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

#1359703

On Thu, Mar 17, 2016 at 11:15 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 10:33 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Mar 17, 2016 at 10:09 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>>> On Thu, Mar 17, 2016 at 10:02 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> From: Miklos Szeredi <mszeredi@redhat.com>
>>>>
>>>> This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
>>>> f_path always point to the overlay and f_inode to the underlay").
>>>>
>>>
>>> Can you put that series in your vfs.git tree?
>>> Easier for getting and testing.
>>
>> Ok, pushed to:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git file_dentry
>>
>
> Is that correct the ext4-related patches 3/4 and 4/4 are Linux-v4.5 material?
> Not applicable fpr v4.4.y-stable?
>

Sorry, for disturbing again - parallelly building some other software
which needs my attention.

Why didn't you split 1/4 into a vfs (generic) part and overlayfs (related) part?
Thinking of bisecting or backporting ovl fixes.

Just my €0,02.

- Sedat -

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

#1359704

On Thu, Mar 17, 2016 at 10:33 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Thu, Mar 17, 2016 at 10:09 AM, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>> On Thu, Mar 17, 2016 at 10:02 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> From: Miklos Szeredi <mszeredi@redhat.com>
>>>
>>> This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
>>> f_path always point to the overlay and f_inode to the underlay").
>>>
>>
>> Can you put that series in your vfs.git tree?
>> Easier for getting and testing.
>
> Ok, pushed to:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git file_dentry
>

Is that correct the ext4-related patches 3/4 and 4/4 are Linux-v4.5 material?
Not applicable fpr v4.4.y-stable?

- Sedat -

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

#1359888

On Thu, Mar 17, 2016 at 11:15:26AM +0100, Sedat Dilek wrote:
> 
> Is that correct the ext4-related patches 3/4 and 4/4 are Linux-v4.5 material?
> Not applicable fpr v4.4.y-stable?

4.5 has already been released, but I think these patches are suitable
for the v4.[345].y-stable branch (which I think is the question you
are really asking).

Al, are you willing to push all of these to Linus before the merge
window closes, with a cc to stable@vger.kernel.org?  Or I can push
them to linus via the ext4.git tree if I get signoffs/acked-by's from
the VFS and NFS maintainers.

I'll want to do a full a regression test cycle since we're pushing
patches created after the merge window opened, but I consider these
bug fixes and will certainly support pushing them to Linus as far as
ext4 is concerned.

Cheers,

					- Ted

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

#1361521

On Thu, Mar 17, 2016 at 10:02:00AM +0100, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@redhat.com>
> 
> This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
> f_path always point to the overlay and f_inode to the underlay").
> 
> Regular files opened on overlayfs will result in the file being opened on
> the underlying filesystem, while f_path points to the overlayfs
> mount/dentry.
> 
> This confuses filesystems which get the dentry from struct file and assume
> it's theirs.
> 
> Add a new helper, file_dentry() [*], to get the filesystem's own dentry
> from the file.  This simply compares file_inode(file->f_path.dentry) to
> file_inode(file) and if they are equal returns file->f_path.dentry (this is
> the common, non-overlayfs case).
> 
> In the uncommon case (regular file on overlayfs) it will call into
> overlayfs's ->d_native_dentry() to get the underlying dentry matching
> file_inode(file).
> 
> [*] If possible, it's better simply to use file_inode() instead.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Tested-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Cc: <stable@vger.kernel.org> # v4.2
> Cc: David Howells <dhowells@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Daniel Axtens <dja@axtens.net>
> ---
>  fs/open.c              | 11 +++++++++++
>  fs/overlayfs/super.c   | 16 ++++++++++++++++
>  include/linux/dcache.h |  1 +
>  include/linux/fs.h     |  2 ++
>  4 files changed, 30 insertions(+)

I have this patch in the ext4.git tree, but I'd like to get an
Acked-by from Al before I send a pull request to Linus.

Al?  Any objections to my sending in this change via the ext4 tree?

						- Ted

> 
> diff --git a/fs/open.c b/fs/open.c
> index 55bdc75e2172..6326c11eda78 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -831,6 +831,17 @@ char *file_path(struct file *filp, char *buf, int buflen)
>  }
>  EXPORT_SYMBOL(file_path);
>  
> +struct dentry *file_dentry(const struct file *file)
> +{
> +	struct dentry *dentry = file->f_path.dentry;
> +
> +	if (likely(d_inode(dentry) == file_inode(file)))
> +		return dentry;
> +	else
> +		return dentry->d_op->d_native_dentry(dentry, file_inode(file));
> +}
> +EXPORT_SYMBOL(file_dentry);
> +
>  /**
>   * vfs_open - open the file at the given path
>   * @path: path to open
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 619ad4b016d2..5142aa2034c4 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
>  	return ret;
>  }
>  
> +static struct dentry *ovl_d_native_dentry(struct dentry *dentry,
> +					 struct inode *inode)
> +{
> +	struct ovl_entry *oe = dentry->d_fsdata;
> +	struct dentry *realentry = ovl_upperdentry_dereference(oe);
> +
> +	if (realentry && inode == d_inode(realentry))
> +		return realentry;
> +	realentry = __ovl_dentry_lower(oe);
> +	if (realentry && inode == d_inode(realentry))
> +		return realentry;
> +	BUG();
> +}
> +
>  static const struct dentry_operations ovl_dentry_operations = {
>  	.d_release = ovl_dentry_release,
>  	.d_select_inode = ovl_d_select_inode,
> +	.d_native_dentry = ovl_d_native_dentry,
>  };
>  
>  static const struct dentry_operations ovl_reval_dentry_operations = {
>  	.d_release = ovl_dentry_release,
>  	.d_select_inode = ovl_d_select_inode,
> +	.d_native_dentry = ovl_d_native_dentry,
>  	.d_revalidate = ovl_dentry_revalidate,
>  	.d_weak_revalidate = ovl_dentry_weak_revalidate,
>  };
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index c4b5f4b3f8f8..99ecb6de636c 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -161,6 +161,7 @@ struct dentry_operations {
>  	struct vfsmount *(*d_automount)(struct path *);
>  	int (*d_manage)(struct dentry *, bool);
>  	struct inode *(*d_select_inode)(struct dentry *, unsigned);
> +	struct dentry *(*d_native_dentry)(struct dentry *, struct inode *);
>  } ____cacheline_aligned;
>  
>  /*
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ae681002100a..1091d9f43271 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(const struct file *f)
>  	return f->f_inode;
>  }
>  
> +extern struct dentry *file_dentry(const struct file *file);
> +
>  static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  {
>  	return locks_lock_inode_wait(file_inode(filp), fl);
> -- 
> 2.1.4
> 

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

#1361524

On Mon, Mar 21, 2016 at 01:02:15AM -0400, Theodore Ts'o wrote:

> I have this patch in the ext4.git tree, but I'd like to get an
> Acked-by from Al before I send a pull request to Linus.
> 
> Al?  Any objections to my sending in this change via the ext4 tree?
> 						- Ted

FWIW, I would rather add DCACHE_OP_REAL (set at d_set_d_op()
time) and turned that into

static inline struct dentry *d_real(const struct dentry *dentry)
{
	if (unlikely(dentry->d_flags & DCACHE_OP_NATIVE_DENTRY))
		returd dentry->d_op->d_real(dentry);
	else
		return dentry;
}
static inline struct dentry *file_dentry(const struct file *file)
{
	return d_real(file->f_path.dentry);
}

and used ovl_dentry_real as ->d_real for overlayfs.  Miklos, do you
see any problems with that variant?

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

#1362347

>> From: Miklos Szeredi <mszeredi@redhat.com>
>> 
>> This series fixes bugs in nfs and ext4 due to 4bacc9c9234c ("overlayfs: Make
>> f_path always point to the overlay and f_inode to the underlay").
>> 
>> Regular files opened on overlayfs will result in the file being opened on
>> the underlying filesystem, while f_path points to the overlayfs
>> mount/dentry.
>> 
>> This confuses filesystems which get the dentry from struct file and assume
>> it's theirs.
>> 
>> Add a new helper, file_dentry() [*], to get the filesystem's own dentry
>> from the file.  This simply compares file_inode(file->f_path.dentry) to
>> file_inode(file) and if they are equal returns file->f_path.dentry (this is
>> the common, non-overlayfs case).
>> 
>> In the uncommon case (regular file on overlayfs) it will call into
>> overlayfs's ->d_native_dentry() to get the underlying dentry matching
>> file_inode(file).
>> 
>> [*] If possible, it's better simply to use file_inode() instead.

Hopefully this is not so late as to be useless!
For this entire series:
Tested-by: Daniel Axtens <dja@axtens.net>

Regards,
Daniel

>> 
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> Tested-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> Reviewed-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> Cc: <stable@vger.kernel.org> # v4.2
>> Cc: David Howells <dhowells@redhat.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Theodore Ts'o <tytso@mit.edu>
>> Cc: Daniel Axtens <dja@axtens.net>
>> ---
>>  fs/open.c              | 11 +++++++++++
>>  fs/overlayfs/super.c   | 16 ++++++++++++++++
>>  include/linux/dcache.h |  1 +
>>  include/linux/fs.h     |  2 ++
>>  4 files changed, 30 insertions(+)
>
> I have this patch in the ext4.git tree, but I'd like to get an
> Acked-by from Al before I send a pull request to Linus.
>
> Al?  Any objections to my sending in this change via the ext4 tree?
>
> 						- Ted
>
>> 
>> diff --git a/fs/open.c b/fs/open.c
>> index 55bdc75e2172..6326c11eda78 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -831,6 +831,17 @@ char *file_path(struct file *filp, char *buf, int buflen)
>>  }
>>  EXPORT_SYMBOL(file_path);
>>  
>> +struct dentry *file_dentry(const struct file *file)
>> +{
>> +	struct dentry *dentry = file->f_path.dentry;
>> +
>> +	if (likely(d_inode(dentry) == file_inode(file)))
>> +		return dentry;
>> +	else
>> +		return dentry->d_op->d_native_dentry(dentry, file_inode(file));
>> +}
>> +EXPORT_SYMBOL(file_dentry);
>> +
>>  /**
>>   * vfs_open - open the file at the given path
>>   * @path: path to open
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 619ad4b016d2..5142aa2034c4 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -336,14 +336,30 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
>>  	return ret;
>>  }
>>  
>> +static struct dentry *ovl_d_native_dentry(struct dentry *dentry,
>> +					 struct inode *inode)
>> +{
>> +	struct ovl_entry *oe = dentry->d_fsdata;
>> +	struct dentry *realentry = ovl_upperdentry_dereference(oe);
>> +
>> +	if (realentry && inode == d_inode(realentry))
>> +		return realentry;
>> +	realentry = __ovl_dentry_lower(oe);
>> +	if (realentry && inode == d_inode(realentry))
>> +		return realentry;
>> +	BUG();
>> +}
>> +
>>  static const struct dentry_operations ovl_dentry_operations = {
>>  	.d_release = ovl_dentry_release,
>>  	.d_select_inode = ovl_d_select_inode,
>> +	.d_native_dentry = ovl_d_native_dentry,
>>  };
>>  
>>  static const struct dentry_operations ovl_reval_dentry_operations = {
>>  	.d_release = ovl_dentry_release,
>>  	.d_select_inode = ovl_d_select_inode,
>> +	.d_native_dentry = ovl_d_native_dentry,
>>  	.d_revalidate = ovl_dentry_revalidate,
>>  	.d_weak_revalidate = ovl_dentry_weak_revalidate,
>>  };
>> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
>> index c4b5f4b3f8f8..99ecb6de636c 100644
>> --- a/include/linux/dcache.h
>> +++ b/include/linux/dcache.h
>> @@ -161,6 +161,7 @@ struct dentry_operations {
>>  	struct vfsmount *(*d_automount)(struct path *);
>>  	int (*d_manage)(struct dentry *, bool);
>>  	struct inode *(*d_select_inode)(struct dentry *, unsigned);
>> +	struct dentry *(*d_native_dentry)(struct dentry *, struct inode *);
>>  } ____cacheline_aligned;
>>  
>>  /*
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index ae681002100a..1091d9f43271 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1234,6 +1234,8 @@ static inline struct inode *file_inode(const struct file *f)
>>  	return f->f_inode;
>>  }
>>  
>> +extern struct dentry *file_dentry(const struct file *file);
>> +
>>  static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>>  {
>>  	return locks_lock_inode_wait(file_inode(filp), fl);
>> -- 
>> 2.1.4
>> 

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

#1361525

On Thu, Mar 17, 2016 at 10:02:00AM +0100, Miklos Szeredi wrote:
> Add a new helper, file_dentry() [*], to get the filesystem's own dentry
> from the file.  This simply compares file_inode(file->f_path.dentry) to
> file_inode(file) and if they are equal returns file->f_path.dentry (this is
> the common, non-overlayfs case).
> 
> In the uncommon case (regular file on overlayfs) it will call into
> overlayfs's ->d_native_dentry() to get the underlying dentry matching
> file_inode(file).

What's wrong with making ovl_dentry_real() an instance of optional
->d_real() method and having a flag (DCACHE_OP_REAL) controlling its
calls?  With d_real(dentry) returning either that or dentry itself,
and file_dentry(file) being simply d_real(file->f_path.dentry)...

Why do we need to look at the inode at all?  d_set_d_op() dereferences
->d_op anyway, as well as setting ->d_flags, so there's no extra cost
there, and "test bit in ->d_flags + branch not taken" is all it would
cost in normal case...

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

#1361589

On Mon, Mar 21, 2016 at 6:28 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Mar 17, 2016 at 10:02:00AM +0100, Miklos Szeredi wrote:
>> Add a new helper, file_dentry() [*], to get the filesystem's own dentry
>> from the file.  This simply compares file_inode(file->f_path.dentry) to
>> file_inode(file) and if they are equal returns file->f_path.dentry (this is
>> the common, non-overlayfs case).
>>
>> In the uncommon case (regular file on overlayfs) it will call into
>> overlayfs's ->d_native_dentry() to get the underlying dentry matching
>> file_inode(file).
>
> What's wrong with making ovl_dentry_real() an instance of optional
> ->d_real() method and having a flag (DCACHE_OP_REAL) controlling its
> calls?  With d_real(dentry) returning either that or dentry itself,
> and file_dentry(file) being simply d_real(file->f_path.dentry)...
>
> Why do we need to look at the inode at all?  d_set_d_op() dereferences
> ->d_op anyway, as well as setting ->d_flags, so there's no extra cost
> there, and "test bit in ->d_flags + branch not taken" is all it would
> cost in normal case...

Checking DCACHE_OP_REAL insted of inode would be fine.

But d_real() needs inode as well.  Consider the case where

 1) file opened on lower layer
 2) copied up
 3) file_dentry() called

d_real(file->f_path.dentry) would return the upper dentry but that's
not what we want.

Thanks,
Miklos

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web