linux-kernel - 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+55aFw99_LwZNsUe3Nep=Gf+N5fj7eOSAr7s2vRDC+N1Yz6Mg@mail.gmail.com>
Date:   Tue, 14 Nov 2017 17:11:47 -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 Tue, Nov 14, 2017 at 4:56 PM, Theodore Ts'o <tytso@....edu> wrote:
>
>    The e2fsprogs user space libraries all use
> EXT4_*_FL, and we can't change that without breaking applications
> depending on userspace, but we can keep things consistent in the
> kernel, and that probably means completely converting away from
> EXT4_*_FL, if possible.

So I don't think you'd necessarily need to convert from one to the
other, but wouldn't it be nice if you at least defined one in terms of
the other, ie something like

  #define EXT4_ENCRYPT_FL (1u << EXT4_INODE_ENCRYPT)

so that when you grep for one you see how they are directly related.

Now it was much less obvious, and I was nervous because that whole
series did introduce _different_ bits that were not in the same space
at all, and encoded the same thing (ie that S_ENCRYPTED bit).

Maybe this normally doesn't come up, but it was not all that obvious,
particularly since there was a lot of indirection:

 ext4_encrypted_inode() ->
    ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT) ->
        EXT4_INODE_BIT_FNS()

That EXT4_INODE_BIT_FNS thing was really fascinating to see.

So just confirming that yes,

   ext4_encrypted_inode()

is the same thing as

   EXT4_I(inode)->i_flags & EXT4_ENCRYPT_FL

was a real adventure.

Making it clear that EXT4_ENCRYPT_FL and EXT4_INODE_ENCRYPT are the
same bit would maybe have lessened the confusion at least a tiny bit.

Of course, not having five different ways to test the same bit would
have been even better.

Ok, I'm exaggerating.

But there really does seem to be a lot of different ways to check
i_flags bits, with some uses checking it directly, the places
_setting_ it using ext4_set_inode_flag(), and then other testers using
bit-specific helper.

And that somewhat confusing model seems to be true of pretty much all the bits.

As long as you can keep track of it, I guess it's fine.

            Linus