All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] vfs: add file_dentry()
@ 2016-03-17  9:02 Miklos Szeredi
  2016-03-17  9:02 ` [PATCH 2/4] nfs: use file_dentry() Miklos Szeredi
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Miklos Szeredi @ 2016-03-17  9:02 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel
  Cc: dhowells, viro, Goldwyn Rodrigues, Trond Myklebust, stable,
	Theodore Ts'o, Daniel Axtens

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] nfs: use file_dentry()
  2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
@ 2016-03-17  9:02 ` Miklos Szeredi
  2016-03-17  9:02 ` [PATCH 3/4] ext4: use dget_parent() in ext4_file_open() Miklos Szeredi
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Miklos Szeredi @ 2016-03-17  9:02 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel
  Cc: dhowells, viro, Goldwyn Rodrigues, Trond Myklebust, stable

From: Miklos Szeredi <mszeredi@redhat.com>

NFS 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.

Fixes: 4bacc9c9234c ("overlayfs: Make f_path always point to the overlay and f_inode to the underlay")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Tested-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Acked-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>
---
 fs/nfs/dir.c      | 6 +++---
 fs/nfs/inode.c    | 2 +-
 fs/nfs/nfs4file.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 9cce67043f92..7ded17764754 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -377,7 +377,7 @@ int nfs_readdir_xdr_filler(struct page **pages, nfs_readdir_descriptor_t *desc,
  again:
 	timestamp = jiffies;
 	gencount = nfs_inc_attr_generation_counter();
-	error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, entry->cookie, pages,
+	error = NFS_PROTO(inode)->readdir(file_dentry(file), cred, entry->cookie, pages,
 					  NFS_SERVER(inode)->dtsize, desc->plus);
 	if (error < 0) {
 		/* We requested READDIRPLUS, but the server doesn't grok it */
@@ -560,7 +560,7 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *en
 		count++;
 
 		if (desc->plus != 0)
-			nfs_prime_dcache(desc->file->f_path.dentry, entry);
+			nfs_prime_dcache(file_dentry(desc->file), entry);
 
 		status = nfs_readdir_add_to_array(entry, page);
 		if (status != 0)
@@ -864,7 +864,7 @@ static bool nfs_dir_mapping_need_revalidate(struct inode *dir)
  */
 static int nfs_readdir(struct file *file, struct dir_context *ctx)
 {
-	struct dentry	*dentry = file->f_path.dentry;
+	struct dentry	*dentry = file_dentry(file);
 	struct inode	*inode = d_inode(dentry);
 	nfs_readdir_descriptor_t my_desc,
 			*desc = &my_desc;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 86faecf8f328..847b678af4f0 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -940,7 +940,7 @@ int nfs_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 	nfs_file_set_open_context(filp, ctx);
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 57ca1c8039c1..2a9ff14cfb3b 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -26,7 +26,7 @@ static int
 nfs4_file_open(struct inode *inode, struct file *filp)
 {
 	struct nfs_open_context *ctx;
-	struct dentry *dentry = filp->f_path.dentry;
+	struct dentry *dentry = file_dentry(filp);
 	struct dentry *parent = NULL;
 	struct inode *dir;
 	unsigned openflags = filp->f_flags;
@@ -57,7 +57,7 @@ nfs4_file_open(struct inode *inode, struct file *filp)
 	parent = dget_parent(dentry);
 	dir = d_inode(parent);
 
-	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	ctx = alloc_nfs_open_context(file_dentry(filp), filp->f_mode);
 	err = PTR_ERR(ctx);
 	if (IS_ERR(ctx))
 		goto out;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] ext4: use dget_parent() in ext4_file_open()
  2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
  2016-03-17  9:02 ` [PATCH 2/4] nfs: use file_dentry() Miklos Szeredi
@ 2016-03-17  9:02 ` Miklos Szeredi
  2016-03-17  9:02 ` [PATCH 4/4] ext4: use file_dentry() Miklos Szeredi
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Miklos Szeredi @ 2016-03-17  9:02 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel; +Cc: dhowells, viro, Theodore Ts'o, stable

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] ext4: use file_dentry()
  2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
  2016-03-17  9:02 ` [PATCH 2/4] nfs: use file_dentry() Miklos Szeredi
  2016-03-17  9:02 ` [PATCH 3/4] ext4: use dget_parent() in ext4_file_open() Miklos Szeredi
@ 2016-03-17  9:02 ` Miklos Szeredi
  2016-03-17  9:09 ` [PATCH 1/4] vfs: add file_dentry() Sedat Dilek
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Miklos Szeredi @ 2016-03-17  9:02 UTC (permalink / raw)
  To: linux-kernel, linux-fsdevel
  Cc: dhowells, viro, Daniel Axtens, Theodore Ts'o, stable

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
                   ` (2 preceding siblings ...)
  2016-03-17  9:02 ` [PATCH 4/4] ext4: use file_dentry() Miklos Szeredi
@ 2016-03-17  9:09 ` Sedat Dilek
  2016-03-17  9:33   ` Miklos Szeredi
  2016-03-21  5:02 ` Theodore Ts'o
  2016-03-21  5:28 ` Al Viro
  5 siblings, 1 reply; 14+ messages in thread
From: Sedat Dilek @ 2016-03-17  9:09 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: LKML, linux-fsdevel, David Howells, Al Viro, Goldwyn Rodrigues,
	Trond Myklebust, stable, Theodore Ts'o, Daniel Axtens

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17  9:09 ` [PATCH 1/4] vfs: add file_dentry() Sedat Dilek
@ 2016-03-17  9:33   ` Miklos Szeredi
  2016-03-17 10:15     ` Sedat Dilek
  0 siblings, 1 reply; 14+ messages in thread
From: Miklos Szeredi @ 2016-03-17  9:33 UTC (permalink / raw)
  To: Sedat Dilek
  Cc: LKML, linux-fsdevel, David Howells, Al Viro, Goldwyn Rodrigues,
	Trond Myklebust, stable, Theodore Ts'o, Daniel Axtens

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17  9:33   ` Miklos Szeredi
@ 2016-03-17 10:15     ` Sedat Dilek
  2016-03-17 10:19       ` Sedat Dilek
  2016-03-17 14:18       ` Theodore Ts'o
  0 siblings, 2 replies; 14+ messages in thread
From: Sedat Dilek @ 2016-03-17 10:15 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: LKML, linux-fsdevel, David Howells, Al Viro, Goldwyn Rodrigues,
	Trond Myklebust, stable, Theodore Ts'o, Daniel Axtens

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 -

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17 10:15     ` Sedat Dilek
@ 2016-03-17 10:19       ` Sedat Dilek
  2016-03-17 14:18       ` Theodore Ts'o
  1 sibling, 0 replies; 14+ messages in thread
From: Sedat Dilek @ 2016-03-17 10:19 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: LKML, linux-fsdevel, David Howells, Al Viro, Goldwyn Rodrigues,
	Trond Myklebust, stable, Theodore Ts'o, Daniel Axtens

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 -

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17 10:15     ` Sedat Dilek
  2016-03-17 10:19       ` Sedat Dilek
@ 2016-03-17 14:18       ` Theodore Ts'o
  1 sibling, 0 replies; 14+ messages in thread
From: Theodore Ts'o @ 2016-03-17 14:18 UTC (permalink / raw)
  To: Sedat Dilek
  Cc: Miklos Szeredi, LKML, linux-fsdevel, David Howells, Al Viro,
	Goldwyn Rodrigues, Trond Myklebust, stable, Daniel Axtens

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
                   ` (3 preceding siblings ...)
  2016-03-17  9:09 ` [PATCH 1/4] vfs: add file_dentry() Sedat Dilek
@ 2016-03-21  5:02 ` Theodore Ts'o
  2016-03-21  5:22   ` Al Viro
  2016-03-22  6:24   ` Daniel Axtens
  2016-03-21  5:28 ` Al Viro
  5 siblings, 2 replies; 14+ messages in thread
From: Theodore Ts'o @ 2016-03-21  5:02 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, dhowells, viro, Goldwyn Rodrigues,
	Trond Myklebust, stable, Daniel Axtens

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
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-21  5:02 ` Theodore Ts'o
@ 2016-03-21  5:22   ` Al Viro
  2016-03-22  6:24   ` Daniel Axtens
  1 sibling, 0 replies; 14+ messages in thread
From: Al Viro @ 2016-03-21  5:22 UTC (permalink / raw)
  To: Theodore Ts'o, Miklos Szeredi, linux-kernel, linux-fsdevel,
	dhowells, Goldwyn Rodrigues, Trond Myklebust, stable,
	Daniel Axtens

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
                   ` (4 preceding siblings ...)
  2016-03-21  5:02 ` Theodore Ts'o
@ 2016-03-21  5:28 ` Al Viro
  2016-03-21  8:16   ` Miklos Szeredi
  5 siblings, 1 reply; 14+ messages in thread
From: Al Viro @ 2016-03-21  5:28 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, dhowells, Goldwyn Rodrigues,
	Trond Myklebust, stable, Theodore Ts'o, Daniel Axtens

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...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-21  5:28 ` Al Viro
@ 2016-03-21  8:16   ` Miklos Szeredi
  0 siblings, 0 replies; 14+ messages in thread
From: Miklos Szeredi @ 2016-03-21  8:16 UTC (permalink / raw)
  To: Al Viro
  Cc: Kernel Mailing List, Linux-Fsdevel, David Howells,
	Goldwyn Rodrigues, Trond Myklebust, stable, Theodore Ts'o,
	Daniel Axtens

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] vfs: add file_dentry()
  2016-03-21  5:02 ` Theodore Ts'o
  2016-03-21  5:22   ` Al Viro
@ 2016-03-22  6:24   ` Daniel Axtens
  1 sibling, 0 replies; 14+ messages in thread
From: Daniel Axtens @ 2016-03-22  6:24 UTC (permalink / raw)
  To: Theodore Ts'o, Miklos Szeredi
  Cc: linux-kernel, linux-fsdevel, dhowells, viro, Goldwyn Rodrigues,
	Trond Myklebust, stable

>> 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
>> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-03-22  6:24 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-17  9:02 [PATCH 1/4] vfs: add file_dentry() Miklos Szeredi
2016-03-17  9:02 ` [PATCH 2/4] nfs: use file_dentry() Miklos Szeredi
2016-03-17  9:02 ` [PATCH 3/4] ext4: use dget_parent() in ext4_file_open() Miklos Szeredi
2016-03-17  9:02 ` [PATCH 4/4] ext4: use file_dentry() Miklos Szeredi
2016-03-17  9:09 ` [PATCH 1/4] vfs: add file_dentry() Sedat Dilek
2016-03-17  9:33   ` Miklos Szeredi
2016-03-17 10:15     ` Sedat Dilek
2016-03-17 10:19       ` Sedat Dilek
2016-03-17 14:18       ` Theodore Ts'o
2016-03-21  5:02 ` Theodore Ts'o
2016-03-21  5:22   ` Al Viro
2016-03-22  6:24   ` Daniel Axtens
2016-03-21  5:28 ` Al Viro
2016-03-21  8:16   ` Miklos Szeredi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.