2016-11-24 19:47:51

by Fabian Frédérick

[permalink] [raw]
Subject: [PATCH 1/1 linux-next] ext4: add compatibility flag check

data=journal mount option should disable O_DIRECT access
(See Documentation/filesystems/ext4.txt) but open operations
using O_CREAT|O_RDWR|O_DIRECT|O_SYNC have no warning in return and file is being
created. This patch adds vfs super_operations compatibility flag function
returning -EPERM in such a case.

Signed-off-by: Fabian Frederick <[email protected]>
---
fs/ext4/super.c | 10 ++++++++++
fs/namei.c | 12 ++++++++++++
include/linux/fs.h | 1 +
3 files changed, 23 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 72b459d..f2cd8e3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1195,6 +1195,15 @@ static struct fscrypt_operations ext4_cryptops = {
};
#endif

+static int ext4_compat_flag_check(struct inode *inode, int openflag)
+{
+ if (openflag & O_DIRECT &&
+ test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)
+ return -EPERM;
+
+ return 0;
+}
+
#ifdef CONFIG_QUOTA
static char *quotatypes[] = INITQFNAMES;
#define QTYPE2NAME(t) (quotatypes[t])
@@ -1267,6 +1276,7 @@ static const struct super_operations ext4_sops = {
.get_dquots = ext4_get_dquots,
#endif
.bdev_try_to_free_page = bdev_try_to_free_page,
+ .compat_flag_check = ext4_compat_flag_check,
};

static const struct export_operations ext4_export_ops = {
diff --git a/fs/namei.c b/fs/namei.c
index 1669c93d..7edbfbb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3493,6 +3493,18 @@ static struct file *path_openat(struct nameidata *nd,
put_filp(file);
return ERR_CAST(s);
}
+
+ if (unlikely(file->f_flags & O_DIRECT)) {
+ struct super_block *sb = nd->inode->i_sb;
+
+ if (unlikely(sb->s_op->compat_flag_check)) {
+ error = sb->s_op->compat_flag_check(nd->inode,
+ op->open_flag);
+ if (error)
+ goto out2;
+ }
+ }
+
while (!(error = link_path_walk(s, nd)) &&
(error = do_last(nd, file, op, &opened)) > 0) {
nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 03a5a39..bba6c0b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1776,6 +1776,7 @@ struct super_operations {
struct shrink_control *);
long (*free_cached_objects)(struct super_block *,
struct shrink_control *);
+ int (*compat_flag_check)(struct inode *, int);
};

/*
--
2.7.4


2016-11-24 20:39:06

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] ext4: add compatibility flag check

On Thu, Nov 24, 2016 at 08:47:41PM +0100, Fabian Frederick wrote:
> data=journal mount option should disable O_DIRECT access
> (See Documentation/filesystems/ext4.txt) but open operations
> using O_CREAT|O_RDWR|O_DIRECT|O_SYNC have no warning in return and file is being
> created. This patch adds vfs super_operations compatibility flag function
> returning -EPERM in such a case.

Why not simply check it in ->open()? Occam's Razor and all such...

2016-11-24 21:44:09

by Fabian Frédérick

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] ext4: add compatibility flag check



> On 24 November 2016 at 21:39 Al Viro <[email protected]> wrote:
>
>
> On Thu, Nov 24, 2016 at 08:47:41PM +0100, Fabian Frederick wrote:
> > data=journal mount option should disable O_DIRECT access
> > (See Documentation/filesystems/ext4.txt) but open operations
> > using O_CREAT|O_RDWR|O_DIRECT|O_SYNC have no warning in return and file is
> > being
> > created. This patch adds vfs super_operations compatibility flag function
> > returning -EPERM in such a case.
>
> Why not simply check it in ->open()?  Occam's Razor and all such...
Thanks for the advice Al but I already tried that solution (see below) and had
an empty file created
(write operation is of course avoided).

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 2a822d3..ec414b5 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -323,6 +323,10 @@ static int ext4_file_open(struct inode * inode, struct file
* filp)
        char buf[64], *cp;
        int ret;
 
+       if ((test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA) &&
+          (filp->f_flags & O_DIRECT ))
+               return -EPERM;
+
        if (unlikely(!(sbi->s_mount_flags & EXT4_MF_MNTDIR_SAMPLED) &&
                     !(sb->s_flags & MS_RDONLY))) {
                sbi->s_mount_flags |= EXT4_MF_MNTDIR_SAMPLED;

2016-11-25 05:27:03

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] ext4: add compatibility flag check

On Thu, Nov 24, 2016 at 08:47:41PM +0100, Fabian Frederick wrote:
> data=journal mount option should disable O_DIRECT access
> (See Documentation/filesystems/ext4.txt) but open operations
> using O_CREAT|O_RDWR|O_DIRECT|O_SYNC have no warning in return and file is being
> created. This patch adds vfs super_operations compatibility flag function
> returning -EPERM in such a case.
>
> Signed-off-by: Fabian Frederick <[email protected]>

The general practice by most file systems in Linux (for better or for
worse) is to silently fall back to buffered I/O instead of failing the
O_DIRECT open. Feel free to try to convince linux-fsdevel otherwise,
but that is the general and historical consensus of Linux file system
developers.

Cheers,

- Ted

2016-11-25 06:14:45

by Fabian Frédérick

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] ext4: add compatibility flag check



> On 25 November 2016 at 06:26 Theodore Ts'o <[email protected]> wrote:
>
>
> On Thu, Nov 24, 2016 at 08:47:41PM +0100, Fabian Frederick wrote:
> > data=journal mount option should disable O_DIRECT access
> > (See Documentation/filesystems/ext4.txt) but open operations
> > using O_CREAT|O_RDWR|O_DIRECT|O_SYNC have no warning in return and file is
> > being
> > created. This patch adds vfs super_operations compatibility flag function
> > returning -EPERM in such a case.
> >
> > Signed-off-by: Fabian Frederick <[email protected]>
>
> The general practice by most file systems in Linux (for better or for
> worse) is to silently fall back to buffered I/O instead of failing the
> O_DIRECT open.  Feel free to try to convince linux-fsdevel otherwise,
> but that is the general and historical consensus of Linux file system
> developers.
>
> Cheers,

Thanks a lot Ted, I'll have a closer look at vfs/ext4 documentation and add some
details if required.

Regards,
Fabian

>
>                                       - Ted