On Thu, Oct 31, 2019 at 03:25:03PM -0700, Eric Biggers wrote:

It's more important to clean up the IS_ENCRYPTED(inode) &&
S_ISREG(inode->i_mode) checks that are duplicated in fs/{ext4,f2fs}/, so I've
been thinking of adding a helper:

static inline bool fscrypt_needs_contents_encryption(const struct inode *inode)
{
        return IS_ENABLED(CONFIG_FS_ENCRYPTION) && IS_ENCRYPTED(inode) &&
               S_ISREG(inode->i_mode);
}

Sounds fine.

I don't think combining these things is a good idea because it would restrict
the use of inline encryption to filesystems that allow IV_INO_LBLK_64 encryption
policies, i.e. filesystems that have stable inode numbers, 32-bit inodes, and
32-bit file logical block numbers.

The on-disk format (i.e. the type of encryption policy chosen) and the
implementation (inline or filesystem-layer crypto) are really two separate
things.  This was one of the changes in v4 => v5 of this patchset; these two
things used to be conflated but now they are separate.  Now you can use inline
encryption with the existing fscrypt policies too.

We could use two separate SB_* flags, like SB_INLINE_CRYPT and
SB_IV_INO_LBLK_64_SUPPORT.

Yes, I think that is a good idea.

However, the ->has_stable_inodes() and
->get_ino_and_lblk_bits() methods are nice because they separate the filesystem
properties from the question of "is this encryption policy supported".
Declaring the filesystem properties is easier to do because it doesn't require
any fscrypt-specific knowledge.  Also, fs/crypto/ could use these properties in
different ways in the future, e.g. if another IV generation scheme is added.

I don't really like writing up method boilerplates for something that
is a simple boolean flag.