Re: [PATCH RFC DRAFT v2 00/13] Move fscrypt and fsverity out of struct inode

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-07-22 at 14:57 +0200, Christian Brauner wrote:
> Hey,
> 
> This is a POC. We're still discussing alternatives and I want to provide
> some useful data on what I learned about using offsets to drop fscrypt
> and fsverity from struct inode.
> 
> As discussed, this moves the fscrypt and fsverity pointers out of struct
> inode shrinking it by 16 bytes. The pointers move into the individual
> filesystems that actually do make use of them.
> 
> In order to find the fscrypt and fsverity data pointers offsets from the
> embedded struct inode in the filesystem's private inode data are
> stored in struct inode_operations. This means we get fast access to the
> data pointers without having to rely on indirect calls.
> 
> Bugs & Issues
> =============
> 
> * For fscrypt specifically the biggest issue is
>   fscrypt_prepare_new_inode() is called in filesystem's inode allocation
>   functions before inode->i_op is set. That means the offset isn't
>   available at the time when we would need it. To fix this we can set
>   dummy encrypted inode operations for the respective filesystem with an
>   initialized offset.
> 
> * For both fscrypt & fsverity the biggest issue is that every codepath
>   that currently calls make_bad_inode() after having initialized fscrypt
>   or fsverity data will override inode->i_op with bad_inode_ops. At
>   which point we're back to the previous problem: The offset isn't
>   available anymore. So when inode->i_sb->s_op->evict_inode() is called
>   fscrypt_put_encryption_info() doesn't have the offset available
>   anymore and would corrupt the hell out of everything and also leak
>   memory.
> 
>   Obviously we could use a flag to detect a bad inodes instead of i_op
>   and let the filesystem assign it's own bad inode operations including
>   the correct offset. Is it worth it?
> 
>   The other way I see we can fix this if we require fixed offsets in the
>   filesystems inode so fscrypt and fsverity always now what offset to
>   calculate. We could use two consecutive pointers at the beginning of
>   the filesystem's inode. Does that always work and is it worth it?
> 

We could store the offsets in the superblock. It's an extra pointer
chase to get to the offset in that case, but presumably it should be in
cache in most cases.

We could even do both -- store it in i_ops and somehow allow falling
back to looking in the superblock when i_ops isn't set or when
make_bad_inode has been called.

> Thanks!
> Christian
> 
> Test results:
> 
> + sudo ./check -g encrypt,verity
> FSTYP         -- ext4
> PLATFORM      -- Linux/x86_64 localhost 6.16.0-rc1-g15c8eb9cdbd3 #267 SMP PREEMPT_DYNAMIC Fri Jun  5 15:58:00 CEST 2015
> MKFS_OPTIONS  -- -F /dev/nvme3n1p6
> MOUNT_OPTIONS -- -o acl,user_xattr /dev/nvme3n1p6 /mnt/scratch
> 
> ext4/024 3s ...  3s
> generic/395 4s ...  4s
> generic/396 3s ...  3s
> generic/397 4s ...  3s
> generic/398 4s ...  4s
> generic/399 39s ...  35s
> generic/419 3s ...  4s
> generic/421 4s ...  4s
> generic/429 14s ...  14s
> generic/435 23s ...  22s
> generic/440 3s ...  4s
> generic/548 10s ...  9s
> generic/549 9s ...  9s
> generic/550       [not run] encryption policy '-c 9 -n 9 -f 0' is unusable; probably missing kernel crypto API support
> generic/572        6s
> generic/573        4s
> generic/574        28s
> generic/575        9s
> generic/576 5s ...  4s
> generic/577        4s
> generic/579        24s
> generic/580 4s ...  4s
> generic/581 10s ...  11s
> generic/582 10s ...  9s
> generic/583 9s ...  9s
> generic/584       [not run] encryption policy '-c 9 -n 9 -v 2 -f 0' is unusable; probably missing kernel crypto API support
> generic/592 10s ...  10s
> generic/593 4s ...  4s
> generic/595 7s ...  7s
> generic/602 9s ...  10s
> generic/613 20s ...  20s
> generic/621 9s ...  9s
> generic/624        3s
> generic/625        3s
> generic/692        5s
> generic/693       [not run] encryption policy '-c 1 -n 10 -v 2 -f 0' is unusable; probably missing kernel crypto API support
> generic/739 17s ...  18s
> Ran: ext4/024 generic/395 generic/396 generic/397 generic/398 generic/399 generic/419 generic/421 generic/429 generic/435 generic/440 generic/548 generic/549 generic/550 generic/572 generic/573 generic/574 generic/575 generic/576 generic/577 generic/579 generic/580 generic/581 generic/582 generic/583 generic/584 generic/592 generic/593 generic/595 generic/602 generic/613 generic/621 generic/624 generic/625 generic/692 generic/693 generic/739
> Not run: generic/550 generic/584 generic/693
> Passed all 37 tests
> 
> ---
> Changes in v2:
> - First full implementation.
> - Link to v1: https://lore.kernel.org/20250715-work-inode-fscrypt-v1-1-aa3ef6f44b6b@kernel.org
> 
> ---
> Christian Brauner (13):
>       fs: add fscrypt offset
>       fs/crypto: use accessors
>       ext4: move fscrypt to filesystem inode
>       ubifs: move fscrypt to filesystem inode
>       f2fs: move fscrypt to filesystem inode
>       ceph: move fscrypt to filesystem inode
>       fs: drop i_crypt_info from struct inode
>       fs: add fsverity offset
>       fs/verity: use accessors
>       btrfs: move fsverity to filesystem inode
>       ext4: move fsverity to filesystem inode
>       f2fs: move fsverity to filesystem inode
>       fs: drop i_verity_info from struct inode
> 
>  fs/btrfs/btrfs_inode.h       |  3 +++
>  fs/btrfs/inode.c             | 20 ++++++++++++++++-
>  fs/ceph/dir.c                |  8 +++++++
>  fs/ceph/inode.c              | 21 ++++++++++++++++++
>  fs/crypto/bio.c              |  2 +-
>  fs/crypto/crypto.c           |  8 +++----
>  fs/crypto/fname.c            |  8 +++----
>  fs/crypto/fscrypt_private.h  |  2 +-
>  fs/crypto/hooks.c            |  2 +-
>  fs/crypto/inline_crypt.c     | 10 ++++-----
>  fs/crypto/keysetup.c         | 27 +++++++++++++----------
>  fs/crypto/policy.c           |  6 ++---
>  fs/ext4/ext4.h               |  9 ++++++++
>  fs/ext4/file.c               |  8 +++++++
>  fs/ext4/ialloc.c             |  2 ++
>  fs/ext4/inode.c              |  1 +
>  fs/ext4/mballoc.c            |  3 +++
>  fs/ext4/namei.c              | 23 ++++++++++++++++++++
>  fs/ext4/super.c              |  6 +++++
>  fs/ext4/symlink.c            | 24 ++++++++++++++++++++
>  fs/f2fs/f2fs.h               |  7 ++++++
>  fs/f2fs/file.c               |  8 +++++++
>  fs/f2fs/inode.c              |  1 +
>  fs/f2fs/namei.c              | 41 ++++++++++++++++++++++++++++++++++
>  fs/f2fs/super.c              |  6 +++++
>  fs/ubifs/dir.c               | 52 ++++++++++++++++++++++++--------------------
>  fs/ubifs/file.c              |  8 +++++++
>  fs/ubifs/super.c             |  8 +++++++
>  fs/ubifs/ubifs.h             |  3 +++
>  fs/verity/enable.c           |  2 +-
>  fs/verity/fsverity_private.h |  2 +-
>  fs/verity/open.c             | 18 +++++++++------
>  fs/verity/verify.c           |  2 +-
>  include/linux/fs.h           | 10 ++-------
>  include/linux/fscrypt.h      | 31 ++++++++++++++++++++++++--
>  include/linux/fsverity.h     | 21 ++++++++++++------
>  include/linux/netfs.h        |  6 +++++
>  37 files changed, 337 insertions(+), 82 deletions(-)
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250715-work-inode-fscrypt-2b63b276e793

-- 
Jeff Layton <jlayton@kernel.org>