2016-03-26 21:09:55

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

I have the following patches in the ext4.git tree which I plan to push
to Linus as bug fixes during this development cycles. Al, are you
happy with Miklos's v2 version of "fs: add file_dentrY()" patch?

Miklos Szeredi (4):
fs: add file_dentry()
nfs: use file_dentry()
ext4: use dget_parent() in ext4_file_open()
ext4: use file_dentry()

Theodore Ts'o (1):
ext4 crypto: use dget_parent() in ext4_d_revalidate()

fs/dcache.c | 5 ++++-
fs/ext4/crypto.c | 12 ++++++++----
fs/ext4/file.c | 12 ++++++++----
fs/nfs/dir.c | 6 +++---
fs/nfs/inode.c | 2 +-
fs/nfs/nfs4file.c | 4 ++--
fs/overlayfs/super.c | 33 +++++++++++++++++++++++++++++++++
include/linux/dcache.h | 10 ++++++++++
include/linux/fs.h | 10 ++++++++++
9 files changed, 79 insertions(+), 15 deletions(-)

--
2.5.0



2016-03-26 21:10:00

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 5/5] ext4 crypto: use dget_parent() in ext4_d_revalidate()

This avoids potential problems caused by a race where the inode gets
renamed out from its parent directory and the parent directory is
deleted while ext4_d_revalidate() is running.

Fixes: 28b4c263961c
Reported-by: Al Viro <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
Cc: [email protected]
---
fs/ext4/crypto.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 012fd32..ea69ce4 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -478,13 +478,16 @@ uint32_t ext4_validate_encryption_key_size(uint32_t mode, uint32_t size)
*/
static int ext4_d_revalidate(struct dentry *dentry, unsigned int flags)
{
- struct inode *dir = d_inode(dentry->d_parent);
- struct ext4_crypt_info *ci = EXT4_I(dir)->i_crypt_info;
+ struct dentry *dir;
+ struct ext4_crypt_info *ci;
int dir_has_key, cached_with_key;

- if (!ext4_encrypted_inode(dir))
+ dir = dget_parent(dentry);
+ if (!ext4_encrypted_inode(d_inode(dir))) {
+ dput(dir);
return 0;
-
+ }
+ ci = EXT4_I(d_inode(dir))->i_crypt_info;
if (ci && ci->ci_keyring_key &&
(ci->ci_keyring_key->flags & ((1 << KEY_FLAG_INVALIDATED) |
(1 << KEY_FLAG_REVOKED) |
@@ -494,6 +497,7 @@ static int ext4_d_revalidate(struct dentry *dentry, unsigned int flags)
/* this should eventually be an flag in d_flags */
cached_with_key = dentry->d_fsdata != NULL;
dir_has_key = (ci != NULL);
+ dput(dir);

/*
* If the dentry was cached without the key, and it is a
--
2.5.0

2016-03-26 21:09:58

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 3/5] ext4: use dget_parent() in ext4_file_open()

From: Miklos Szeredi <[email protected]>

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 <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
Cc: <[email protected]> # 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 6659e21..257118d 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -329,7 +329,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;
@@ -373,14 +373,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.5.0

2016-03-26 21:09:59

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 4/5] ext4: use file_dentry()

From: Miklos Szeredi <[email protected]>

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 <[email protected]>
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 <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
Cc: David Howells <[email protected]>
Cc: Al Viro <[email protected]>
Cc: <[email protected]> # 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 257118d..edba9fb 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -374,7 +374,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.5.0

2016-03-26 21:10:18

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 1/5] fs: add file_dentry()

From: Miklos Szeredi <[email protected]>

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 checks file->f_path.dentry->d_flags against
DCACHE_OP_REAL, and returns file->f_path.dentry if DCACHE_OP_REAL is not
set (this is the common, non-overlayfs case).

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

The reason we need to check against the inode is that if the file is copied
up while being open, d_real() would return the upper dentry, while the open
file comes from the lower dentry.

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

Signed-off-by: Miklos Szeredi <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
Tested-by: Goldwyn Rodrigues <[email protected]>
Reviewed-by: Trond Myklebust <[email protected]>
Cc: <[email protected]> # v4.2
Cc: David Howells <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Daniel Axtens <[email protected]>
---
fs/dcache.c | 5 ++++-
fs/overlayfs/super.c | 33 +++++++++++++++++++++++++++++++++
include/linux/dcache.h | 10 ++++++++++
include/linux/fs.h | 10 ++++++++++
4 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 32ceae3..d5ecc6e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1667,7 +1667,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
DCACHE_OP_REVALIDATE |
DCACHE_OP_WEAK_REVALIDATE |
DCACHE_OP_DELETE |
- DCACHE_OP_SELECT_INODE));
+ DCACHE_OP_SELECT_INODE |
+ DCACHE_OP_REAL));
dentry->d_op = op;
if (!op)
return;
@@ -1685,6 +1686,8 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
dentry->d_flags |= DCACHE_OP_PRUNE;
if (op->d_select_inode)
dentry->d_flags |= DCACHE_OP_SELECT_INODE;
+ if (op->d_real)
+ dentry->d_flags |= DCACHE_OP_REAL;

}
EXPORT_SYMBOL(d_set_d_op);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index ef64984..5d972e6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -295,6 +295,37 @@ static void ovl_dentry_release(struct dentry *dentry)
}
}

+static struct dentry *ovl_d_real(struct dentry *dentry, struct inode *inode)
+{
+ struct dentry *real;
+
+ if (d_is_dir(dentry)) {
+ if (!inode || inode == d_inode(dentry))
+ return dentry;
+ goto bug;
+ }
+
+ real = ovl_dentry_upper(dentry);
+ if (real && (!inode || inode == d_inode(real)))
+ return real;
+
+ real = ovl_dentry_lower(dentry);
+ if (!real)
+ goto bug;
+
+ if (!inode || inode == d_inode(real))
+ return real;
+
+ /* Handle recursion */
+ if (real->d_flags & DCACHE_OP_REAL)
+ return real->d_op->d_real(real, inode);
+
+bug:
+ WARN(1, "ovl_d_real(%pd4, %s:%lu\n): real dentry not found\n", dentry,
+ inode ? inode->i_sb->s_id : "NULL", inode ? inode->i_ino : 0);
+ return dentry;
+}
+
static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags)
{
struct ovl_entry *oe = dentry->d_fsdata;
@@ -339,11 +370,13 @@ static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
static const struct dentry_operations ovl_dentry_operations = {
.d_release = ovl_dentry_release,
.d_select_inode = ovl_d_select_inode,
+ .d_real = ovl_d_real,
};

static const struct dentry_operations ovl_reval_dentry_operations = {
.d_release = ovl_dentry_release,
.d_select_inode = ovl_d_select_inode,
+ .d_real = ovl_d_real,
.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 7cb043d..4bb4de8 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_real)(struct dentry *, struct inode *);
} ____cacheline_aligned;

/*
@@ -229,6 +230,7 @@ struct dentry_operations {
#define DCACHE_OP_SELECT_INODE 0x02000000 /* Unioned entry: dcache op selects inode */

#define DCACHE_ENCRYPTED_WITH_KEY 0x04000000 /* dir is encrypted with a valid key */
+#define DCACHE_OP_REAL 0x08000000

extern seqlock_t rename_lock;

@@ -555,4 +557,12 @@ static inline struct dentry *d_backing_dentry(struct dentry *upper)
return upper;
}

+static inline struct dentry *d_real(struct dentry *dentry)
+{
+ if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
+ return dentry->d_op->d_real(dentry, NULL);
+ else
+ return dentry;
+}
+
#endif /* __LINUX_DCACHE_H */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 35d9926..b2ed231 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1241,6 +1241,16 @@ static inline struct inode *file_inode(const struct file *f)
return f->f_inode;
}

+static inline struct dentry *file_dentry(const struct file *file)
+{
+ struct dentry *dentry = file->f_path.dentry;
+
+ if (unlikely(dentry->d_flags & DCACHE_OP_REAL))
+ return dentry->d_op->d_real(dentry, file_inode(file));
+ else
+ return dentry;
+}
+
static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
{
return locks_lock_inode_wait(file_inode(filp), fl);
--
2.5.0


2016-03-26 21:10:18

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 2/5] nfs: use file_dentry()

From: Miklos Szeredi <[email protected]>

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 <[email protected]>
Tested-by: Goldwyn Rodrigues <[email protected]>
Acked-by: Trond Myklebust <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
Cc: <[email protected]> # v4.2
Cc: David Howells <[email protected]>
Cc: Al Viro <[email protected]>
---
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 4bfa7d8..a89d32a 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 86faecf..847b678 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 57ca1c8..2a9ff14 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.5.0


2016-03-27 08:02:46

by Sedat Dilek

[permalink] [raw]
Subject: Re: [PATCH 5/5] ext4 crypto: use dget_parent() in ext4_d_revalidate()

On Sat, Mar 26, 2016 at 10:10 PM, Theodore Ts'o <[email protected]> wrote:
> This avoids potential problems caused by a race where the inode gets
> renamed out from its parent directory and the parent directory is
> deleted while ext4_d_revalidate() is running.
>
> Fixes: 28b4c263961c

Full Fixes-tag...

Fixes: 28b4c263961c ("ext4 crypto: revalidate dentry after adding or
removing the key")

> Reported-by: Al Viro <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> Cc: [email protected]

This for Linux v4.6(-rc1+) ?

- Sedat -

> ---
> fs/ext4/crypto.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> index 012fd32..ea69ce4 100644
> --- a/fs/ext4/crypto.c
> +++ b/fs/ext4/crypto.c
> @@ -478,13 +478,16 @@ uint32_t ext4_validate_encryption_key_size(uint32_t mode, uint32_t size)
> */
> static int ext4_d_revalidate(struct dentry *dentry, unsigned int flags)
> {
> - struct inode *dir = d_inode(dentry->d_parent);
> - struct ext4_crypt_info *ci = EXT4_I(dir)->i_crypt_info;
> + struct dentry *dir;
> + struct ext4_crypt_info *ci;
> int dir_has_key, cached_with_key;
>
> - if (!ext4_encrypted_inode(dir))
> + dir = dget_parent(dentry);
> + if (!ext4_encrypted_inode(d_inode(dir))) {
> + dput(dir);
> return 0;
> -
> + }
> + ci = EXT4_I(d_inode(dir))->i_crypt_info;
> if (ci && ci->ci_keyring_key &&
> (ci->ci_keyring_key->flags & ((1 << KEY_FLAG_INVALIDATED) |
> (1 << KEY_FLAG_REVOKED) |
> @@ -494,6 +497,7 @@ static int ext4_d_revalidate(struct dentry *dentry, unsigned int flags)
> /* this should eventually be an flag in d_flags */
> cached_with_key = dentry->d_fsdata != NULL;
> dir_has_key = (ci != NULL);
> + dput(dir);
>
> /*
> * If the dentry was cached without the key, and it is a
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-03-27 08:06:14

by Sedat Dilek

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Sat, Mar 26, 2016 at 10:09 PM, Theodore Ts'o <[email protected]> wrote:
> I have the following patches in the ext4.git tree which I plan to push
> to Linus as bug fixes during this development cycles. Al, are you
> happy with Miklos's v2 version of "fs: add file_dentrY()" patch?
>
> Miklos Szeredi (4):
> fs: add file_dentry()
> nfs: use file_dentry()
> ext4: use dget_parent() in ext4_file_open()
> ext4: use file_dentry()
>
> Theodore Ts'o (1):
> ext4 crypto: use dget_parent() in ext4_d_revalidate()
>

I asked that already in a previous series of that patchset.

Are those CC-stable-tags correct?

fs: add file_dentry()
Cc: <[email protected]> # v4.2

nfs: use file_dentry()
Cc: <[email protected]> # v4.2

ext4: use dget_parent() in ext4_file_open()
Cc: <[email protected]> # v4.5

ext4: use file_dentry()
Cc: <[email protected]> # v4.5

ext4 crypto: use dget_parent() in ext4_d_revalidate()
Cc: [email protected]

- Sedat -

> fs/dcache.c | 5 ++++-
> fs/ext4/crypto.c | 12 ++++++++----
> fs/ext4/file.c | 12 ++++++++----
> fs/nfs/dir.c | 6 +++---
> fs/nfs/inode.c | 2 +-
> fs/nfs/nfs4file.c | 4 ++--
> fs/overlayfs/super.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/dcache.h | 10 ++++++++++
> include/linux/fs.h | 10 ++++++++++
> 9 files changed, 79 insertions(+), 15 deletions(-)
>
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-03-27 18:15:15

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Sun, Mar 27, 2016 at 10:06:14AM +0200, Sedat Dilek wrote:
> On Sat, Mar 26, 2016 at 10:09 PM, Theodore Ts'o <[email protected]> wrote:
> > I have the following patches in the ext4.git tree which I plan to push
> > to Linus as bug fixes during this development cycles. Al, are you
> > happy with Miklos's v2 version of "fs: add file_dentrY()" patch?
> >
> > Miklos Szeredi (4):
> > fs: add file_dentry()
> > nfs: use file_dentry()
> > ext4: use dget_parent() in ext4_file_open()
> > ext4: use file_dentry()
> >
> > Theodore Ts'o (1):
> > ext4 crypto: use dget_parent() in ext4_d_revalidate()
> >
>
> I asked that already in a previous series of that patchset.
>
> Are those CC-stable-tags correct?
>
> fs: add file_dentry()
> Cc: <[email protected]> # v4.2

To quote from the commit description

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

% git tag --contains 4bacc9c9234c | grep ^v | head -1
v4.2

Cheers,

- Ted

2016-03-27 18:16:01

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 5/5] ext4 crypto: use dget_parent() in ext4_d_revalidate()

On Sun, Mar 27, 2016 at 10:02:46AM +0200, Sedat Dilek wrote:
> On Sat, Mar 26, 2016 at 10:10 PM, Theodore Ts'o <[email protected]> wrote:
> > This avoids potential problems caused by a race where the inode gets
> > renamed out from its parent directory and the parent directory is
> > deleted while ext4_d_revalidate() is running.
> >
> > Fixes: 28b4c263961c
>
> Full Fixes-tag...
>
> Fixes: 28b4c263961c ("ext4 crypto: revalidate dentry after adding or
> removing the key")
>
> > Reported-by: Al Viro <[email protected]>
> > Signed-off-by: Theodore Ts'o <[email protected]>
> > Cc: [email protected]
>
> This for Linux v4.6(-rc1+) ?

Probably v4.6-rc2 at this point, since I'm still waiting for acks from
folks.

- Ted

2016-03-27 19:31:58

by Sedat Dilek

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Sun, Mar 27, 2016 at 8:15 PM, Theodore Ts'o <[email protected]> wrote:
> On Sun, Mar 27, 2016 at 10:06:14AM +0200, Sedat Dilek wrote:
>> On Sat, Mar 26, 2016 at 10:09 PM, Theodore Ts'o <[email protected]> wrote:
>> > I have the following patches in the ext4.git tree which I plan to push
>> > to Linus as bug fixes during this development cycles. Al, are you
>> > happy with Miklos's v2 version of "fs: add file_dentrY()" patch?
>> >
>> > Miklos Szeredi (4):
>> > fs: add file_dentry()
>> > nfs: use file_dentry()
>> > ext4: use dget_parent() in ext4_file_open()
>> > ext4: use file_dentry()
>> >
>> > Theodore Ts'o (1):
>> > ext4 crypto: use dget_parent() in ext4_d_revalidate()
>> >
>>
>> I asked that already in a previous series of that patchset.
>>
>> Are those CC-stable-tags correct?
>>
>> fs: add file_dentry()
>> Cc: <[email protected]> # v4.2
>
> To quote from the commit description
>
> 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").
>
> % git tag --contains 4bacc9c9234c | grep ^v | head -1
> v4.2
>

v4.2, OK.

fs: add file_dentry()
Cc: <[email protected]> # v4.2

nfs: use file_dentry()
Cc: <[email protected]> # v4.2

v4.5 ?

ext4: use file_dentry()
Cc: <[email protected]> # v4.5

- Sedat -

2016-03-27 22:51:22

by Sedat Dilek

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Sat, Mar 26, 2016 at 10:09 PM, Theodore Ts'o <[email protected]> wrote:
> I have the following patches in the ext4.git tree which I plan to push
> to Linus as bug fixes during this development cycles. Al, are you
> happy with Miklos's v2 version of "fs: add file_dentrY()" patch?
>
> Miklos Szeredi (4):
> fs: add file_dentry()
> nfs: use file_dentry()
> ext4: use dget_parent() in ext4_file_open()
> ext4: use file_dentry()
>
> Theodore Ts'o (1):
> ext4 crypto: use dget_parent() in ext4_d_revalidate()
>
> fs/dcache.c | 5 ++++-
> fs/ext4/crypto.c | 12 ++++++++----
> fs/ext4/file.c | 12 ++++++++----
> fs/nfs/dir.c | 6 +++---
> fs/nfs/inode.c | 2 +-
> fs/nfs/nfs4file.c | 4 ++--
> fs/overlayfs/super.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/dcache.h | 10 ++++++++++
> include/linux/fs.h | 10 ++++++++++
> 9 files changed, 79 insertions(+), 15 deletions(-)
>
> --
> 2.5.0
>

Might be good to have...

"Btrfs: fix crash/invalid memory access on fsync when using overlayfs"

...also in this series.

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/log/?h=overlayfs-next
[2] http://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=e3608e3f68f6e82113555c873333722362eca6a0

2016-03-28 12:36:16

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Sun, Mar 27, 2016 at 09:31:58PM +0200, Sedat Dilek wrote:
> v4.5 ?
>
> ext4: use file_dentry()
> Cc: <[email protected]> # v4.5

This only became a problem in:

Fixes: ff978b09f973 ("ext4 crypto: move context consistency check to ext4_file_open()")

which showed up in v4.5. In fact that commit won't apply previous to
4.5.

- Ted

2016-03-28 14:02:22

by Chris Mason

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Mon, Mar 28, 2016 at 12:51:22AM +0200, Sedat Dilek wrote:
> On Sat, Mar 26, 2016 at 10:09 PM, Theodore Ts'o <[email protected]> wrote:
> > I have the following patches in the ext4.git tree which I plan to push
> > to Linus as bug fixes during this development cycles. Al, are you
> > happy with Miklos's v2 version of "fs: add file_dentrY()" patch?
> >
> > Miklos Szeredi (4):
> > fs: add file_dentry()
> > nfs: use file_dentry()
> > ext4: use dget_parent() in ext4_file_open()
> > ext4: use file_dentry()
> >
> > Theodore Ts'o (1):
> > ext4 crypto: use dget_parent() in ext4_d_revalidate()
> >
> > fs/dcache.c | 5 ++++-
> > fs/ext4/crypto.c | 12 ++++++++----
> > fs/ext4/file.c | 12 ++++++++----
> > fs/nfs/dir.c | 6 +++---
> > fs/nfs/inode.c | 2 +-
> > fs/nfs/nfs4file.c | 4 ++--
> > fs/overlayfs/super.c | 33 +++++++++++++++++++++++++++++++++
> > include/linux/dcache.h | 10 ++++++++++
> > include/linux/fs.h | 10 ++++++++++
> > 9 files changed, 79 insertions(+), 15 deletions(-)
> >
> > --
> > 2.5.0
> >
>
> Might be good to have...
>
> "Btrfs: fix crash/invalid memory access on fsync when using overlayfs"
>
> ...also in this series.

Yeah, I've been waiting on Miklos' patch to push the corresponding btrfs
fix. I can do it separately or it can go in here too. Either way is
fine with me.

-chris

2016-03-29 17:15:23

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Mon, Mar 28, 2016 at 10:02:22AM -0400, Chris Mason wrote:
> >
> > Might be good to have...
> >
> > "Btrfs: fix crash/invalid memory access on fsync when using overlayfs"
> >
> > ...also in this series.
>
> Yeah, I've been waiting on Miklos' patch to push the corresponding btrfs
> fix. I can do it separately or it can go in here too. Either way is
> fine with me.

I wasn't cc'ed on this patch and the only version I can find has a
mangled e-mail address for Filipe Manana. If you can send me the
patch with your S-o-B, I'd be happy to include it in a push to Linus.

Cheers,

- Ted

2016-03-29 19:58:45

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Sat, Mar 26, 2016 at 05:09:55PM -0400, Theodore Ts'o wrote:
> I have the following patches in the ext4.git tree which I plan to push
> to Linus as bug fixes during this development cycles. Al, are you
> happy with Miklos's v2 version of "fs: add file_dentrY()" patch?

I'm not really happy, but I guess we'll have to live with that approach.
I would still like to point out that *any* use of file_dentry() (or
file->f_path.dentry, for that matter) is a serious red flag - odds are,
the code using it is broken, possibly by design.

I still don't understand the locking in ext4 crypto, and I'm not at all
convinced that it is correct ;-/ OTOH, there are filesystems where we
really need dentry and (hopefully) treat it sanely enough, so consider
that helper and method ACKed. I can take it via vfs.git, or leave it
to ext4.git, or do some combination thereof (e.g. the infrastructure
goes into never-rebased branch in vfs.git, with ext4/btrfs/nfs merging
from it). Up to you...

Al, still bloody unhappy about the whole pile of worms...

2016-03-29 20:12:55

by Chris Mason

[permalink] [raw]
Subject: Re: [PATCH 0/5] Miklos's vfs/nfs/ext4 patches in the ext4.git tree

On Tue, Mar 29, 2016 at 01:15:23PM -0400, Theodore Ts'o wrote:
> On Mon, Mar 28, 2016 at 10:02:22AM -0400, Chris Mason wrote:
> > >
> > > Might be good to have...
> > >
> > > "Btrfs: fix crash/invalid memory access on fsync when using overlayfs"
> > >
> > > ...also in this series.
> >
> > Yeah, I've been waiting on Miklos' patch to push the corresponding btrfs
> > fix. I can do it separately or it can go in here too. Either way is
> > fine with me.
>
> I wasn't cc'ed on this patch and the only version I can find has a
> mangled e-mail address for Filipe Manana. If you can send me the
> patch with your S-o-B, I'd be happy to include it in a push to Linus.

Thanks Ted, it'll be easiest to have all the deps in one place.

https://patchwork.kernel.org/patch/8634801/mbox/

(I can forward a copy if that doesn't work)

chris