linux-ext4 - Re: [GIT PULL] ext4 updates for 4.15

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFxbTtsf-WUhH0uY2=dzrn7BjsNuqFT0POD+pViyMwQBfg@mail.gmail.com>
Date:   Tue, 14 Nov 2017 12:59:17 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Theodore Ts'o" <tytso@....edu>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: [GIT PULL] ext4 updates for 4.15

On Mon, Nov 13, 2017 at 8:25 AM, Theodore Ts'o <tytso@....edu> wrote:
> I forgot to mention, there's a merge conflict when pulling the ext4
> and fscrypt trees.  The fixup is relatively straightforward:

It doesn't actually look all that straightforward, and in particular,
the resolution you sent me doesn't actually seem correct:

>                 new_fl |= S_NOATIME;
>         if (flags & EXT4_DIRSYNC_FL)
>                 new_fl |= S_DIRSYNC;
> -       if (test_opt(inode->i_sb, DAX) && S_ISREG(inode->i_mode) &&
> -           !ext4_should_journal_data(inode) && !ext4_has_inline_data(inode) &&
> -           !(flags & EXT4_ENCRYPT_FL))
> +       if (ext4_should_use_dax(inode))
>                 new_fl |= S_DAX;

This now loses the "!(flags & EXT4_ENCRYPT_FL)" test when it sets S_DAX.

Yes, in ext4_should_use_dax(), it has this code

        if (ext4_encrypted_inode(inode))
                return false;

but that test was what commit 2ee6a576be56 changed in favor of just
checking !(flags & EXT4_ENCRYPT_FL).

So that suggested merge resolkution actually undoes some of that
commit 2ee6a576be56.

Of course,

        (flags & EXT4_ENCRYPT_FL)

_should_ be the same as

        ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT);

so It does seem to be harmless, but it's a bit dodgy.

I'll do that suggested resolution, but I have to say that the ext4 bit
testing is incredibly broken and non-obvious. Just as an example:

  fs/ext4/ext4.h:#define EXT4_ENCRYPT_FL                  0x00000800
/* encrypted file */
  fs/ext4/ext4.h: EXT4_INODE_ENCRYPT      = 11,   /* Encrypted file */

yeah, it's the same bit, but it sure as hell isn't obvious. Why the
two totally different ways to define that data?

            Linus