| Message ID | 20201005073606.1949772-1-satyat@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add support for metadata encryption to F2FS | expand |
On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote: > This patch series adds support for metadata encryption to F2FS using > blk-crypto. > > Patch 1 replaces fscrypt_get_devices (which took an array of request_queues > and filled it up) with fscrypt_get_device, which takes a index of the > desired device and returns the device at that index (so the index passed > to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1) > inclusive). This allows callers to avoid having to allocate an array to > pass to fscrypt_get_devices() when they only need to iterate through > each element in the array (and have no use for the array itself). > > Patch 2 introduces some functions to fscrypt that help filesystems perform > metadata encryption. Any filesystem that wants to use metadata encryption > can call fscrypt_setup_metadata_encryption() with the super_block of the > filesystem, the encryption algorithm and the descriptor of the encryption > key. The descriptor is looked up in the logon keyring of the current > session with "fscrypt:" as the prefix of the descriptor. > > The patch also introduces fscrypt_metadata_crypt_bio() which an FS should > call on a bio that the FS wants metadata crypted. The function will add > an encryption context with the metadata encryption key set up by the call > to the above mentioned fscrypt_setup_metadata_encryption(). > > The patch also introduces fscrypt_metadata_crypt_prepare_all_devices(). > Filesystems that use multiple devices should call this function once all > the underlying devices have been determined. An FS might only be able to > determine all the underlying devices after some initial processing that > might already require metadata en/decryption, which is why this function > is separate from fscrypt_setup_metadata_encryption(). > > Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS > will encrypt every block (that's not being encrypted by some other > encryption key, e.g. a per-file key) with the metadata encryption key > except the superblock (and the redundant copy of the superblock). The DUN > of a block is the offset of the block from the start of the F2FS > filesystem. > > Please refer to the commit message for why the superblock was excluded from > en/decryption, and other limitations. The superblock and its copy are > stored in plaintext on disk. The encryption algorithm used for metadata > encryption is stored within the superblock itself. Changes to the userspace > tools (that are required to test out metadata encryption with F2FS) are > also being sent out - I'll post a link as a reply to this mail once it's > out. The userspace patches are at https://lore.kernel.org/linux-fscrypt/20201005074133.1958633-2-satyat@google.com/ > > Satya Tangirala (3): > fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device > fscrypt: Add metadata encryption support > f2fs: Add metadata encryption support > > Documentation/filesystems/f2fs.rst | 12 ++ > fs/crypto/Kconfig | 6 + > fs/crypto/Makefile | 1 + > fs/crypto/fscrypt_private.h | 19 +++ > fs/crypto/inline_crypt.c | 37 +---- > fs/crypto/metadata_crypt.c | 220 +++++++++++++++++++++++++++++ > fs/f2fs/data.c | 24 ++-- > fs/f2fs/f2fs.h | 2 + > fs/f2fs/super.c | 83 +++++++++-- > include/linux/f2fs_fs.h | 3 +- > include/linux/fs.h | 3 + > include/linux/fscrypt.h | 51 ++++++- > 12 files changed, 410 insertions(+), 51 deletions(-) > create mode 100644 fs/crypto/metadata_crypt.c > > -- > 2.28.0.806.g8561365e88-goog >
On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote: > This patch series adds support for metadata encryption to F2FS using > blk-crypto. This patch series needs more explanation about what "metadata encryption" is, why people will want to use it (as opposed to either not using it, or using fscrypt + dm-crypt instead), and why this is the best implementation of it. > Patch 2 introduces some functions to fscrypt that help filesystems perform > metadata encryption. Any filesystem that wants to use metadata encryption > can call fscrypt_setup_metadata_encryption() with the super_block of the > filesystem, the encryption algorithm and the descriptor of the encryption > key. The descriptor is looked up in the logon keyring of the current > session with "fscrypt:" as the prefix of the descriptor. I notice this is missing the step I suggested to include the metadata encryption key in the HKDF application-specific info string when deriving subkeys from the fscrypt master keys. The same effect could also be achieved by adding an additional level to the key hierarchy: each HKDF key would be derived from a fscrypt master key and the metadata encryption key. We need one of those, to guarantee that the file contents encryption is at least as strong as the "metadata encryption". - Eric
On Wed, Oct 07, 2020 at 02:00:40PM -0700, Eric Biggers wrote: > On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote: > > This patch series adds support for metadata encryption to F2FS using > > blk-crypto. > > This patch series needs more explanation about what "metadata encryption" is, > why people will want to use it (as opposed to either not using it, or using > fscrypt + dm-crypt instead), and why this is the best implementation of it. > Sure, I'll add that in the next version > > Patch 2 introduces some functions to fscrypt that help filesystems perform > > metadata encryption. Any filesystem that wants to use metadata encryption > > can call fscrypt_setup_metadata_encryption() with the super_block of the > > filesystem, the encryption algorithm and the descriptor of the encryption > > key. The descriptor is looked up in the logon keyring of the current > > session with "fscrypt:" as the prefix of the descriptor. > > I notice this is missing the step I suggested to include the metadata encryption > key in the HKDF application-specific info string when deriving subkeys from the > fscrypt master keys. > > The same effect could also be achieved by adding an additional level to the key > hierarchy: each HKDF key would be derived from a fscrypt master key and the > metadata encryption key. > > We need one of those, to guarantee that the file contents encryption is at least > as strong as the "metadata encryption". > Yes - I didn't get around to that in the first version, but I'll add that too in the next version. I was going to go with the first approach before I saw your comment - is there one method you'd recommend going with over the other? > - Eric
On Wed, Oct 07, 2020 at 10:05:00PM +0000, Satya Tangirala wrote: > > I notice this is missing the step I suggested to include the metadata encryption > > key in the HKDF application-specific info string when deriving subkeys from the > > fscrypt master keys. > > > > The same effect could also be achieved by adding an additional level to the key > > hierarchy: each HKDF key would be derived from a fscrypt master key and the > > metadata encryption key. > > > > We need one of those, to guarantee that the file contents encryption is at least > > as strong as the "metadata encryption". > > > Yes - I didn't get around to that in the first version, but I'll add > that too in the next version. I was going to go with the first approach > before I saw your comment - is there one method you'd recommend going > with over the other? I'm not entirely sure, but I'm now leaning towards the second approach because it would avoid adding additional work (another SHA-512 block) to all later key derivations. Also it would avoid having to add a super_block argument to fscrypt_hkdf_expand(). But please ask Paul Crowley for his suggestion too. Here's a quick untested patch to consider: diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index dca254590a70..67f8ba3098d3 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -319,6 +319,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key, #define HKDF_CONTEXT_DIRHASH_KEY 5 /* info=file_nonce */ #define HKDF_CONTEXT_IV_INO_LBLK_32_KEY 6 /* info=mode_num||fs_uuid */ #define HKDF_CONTEXT_INODE_HASH_KEY 7 /* info=<empty> */ +#define HKDF_CONTEXT_MIX_METADATA_KEY 8 /* info=metadata_key */ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, const u8 *info, unsigned int infolen, @@ -600,6 +601,20 @@ int fscrypt_setup_v1_file_key(struct fscrypt_info *ci, int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci); +/* metadata_crypt.c */ + +#ifdef CONFIG_FS_ENCRYPTION_METADATA +int fscrypt_mix_in_metadata_key(struct super_block *sb, + struct fscrypt_master_key_secret *secret); +#else +static inline int +fscrypt_mix_in_metadata_key(struct super_block *sb, + struct fscrypt_master_key_secret *secret) +{ + return 0; +} +#endif + /* policy.c */ bool fscrypt_policies_equal(const union fscrypt_policy *policy1, diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c index 0cba7928446d..61d1f0aa802e 100644 --- a/fs/crypto/hkdf.c +++ b/fs/crypto/hkdf.c @@ -174,4 +174,5 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context, void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf) { crypto_free_shash(hkdf->hmac_tfm); + hkdf->hmac_tfm = NULL; } diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c index e74f239c4428..43453a7f77b1 100644 --- a/fs/crypto/keyring.c +++ b/fs/crypto/keyring.c @@ -494,6 +494,10 @@ static int add_master_key(struct super_block *sb, */ memzero_explicit(secret->raw, secret->size); + err = fscrypt_mix_in_metadata_key(sb, secret); + if (err) + return err; + /* Calculate the key identifier */ err = fscrypt_hkdf_expand(&secret->hkdf, HKDF_CONTEXT_KEY_IDENTIFIER, NULL, 0, diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c index 5e16df130509..233e68c35504 100644 --- a/fs/crypto/metadata_crypt.c +++ b/fs/crypto/metadata_crypt.c @@ -13,6 +13,32 @@ #include "fscrypt_private.h" +/* TODO: add comment */ +int fscrypt_mix_in_metadata_key(struct super_block *sb, + struct fscrypt_master_key_secret *secret) +{ + u8 real_key[FSCRYPT_MAX_KEY_SIZE]; + int err; + + if (WARN_ON(secret->size > sizeof(real_key))) + return -EINVAL; + + if (!sb->s_metadata_key) + return 0; + + err = fscrypt_hkdf_expand(&secret->hkdf, HKDF_CONTEXT_MIX_METADATA_KEY, + sb->s_metadata_key->raw, + sb->s_metadata_key->size, + real_key, secret->size); + if (err) + return err; + + fscrypt_destroy_hkdf(&secret->hkdf); + err = fscrypt_init_hkdf(&secret->hkdf, real_key, secret->size); + memzero_explicit(real_key, secret->size); + return err; +} + /* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */ static int fscrypt_metadata_get_key_from_id(const char *prefix, char *descriptor_hex,
On 2020/10/5 15:36, Satya Tangirala wrote: > This patch series adds support for metadata encryption to F2FS using > blk-crypto. It looks this implementation is based on hardware crypto engine, could you please add this info into f2fs.rst as well like inlinecrypt... > > Patch 1 replaces fscrypt_get_devices (which took an array of request_queues > and filled it up) with fscrypt_get_device, which takes a index of the > desired device and returns the device at that index (so the index passed > to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1) > inclusive). This allows callers to avoid having to allocate an array to > pass to fscrypt_get_devices() when they only need to iterate through > each element in the array (and have no use for the array itself). > > Patch 2 introduces some functions to fscrypt that help filesystems perform > metadata encryption. Any filesystem that wants to use metadata encryption > can call fscrypt_setup_metadata_encryption() with the super_block of the > filesystem, the encryption algorithm and the descriptor of the encryption > key. The descriptor is looked up in the logon keyring of the current > session with "fscrypt:" as the prefix of the descriptor. > > The patch also introduces fscrypt_metadata_crypt_bio() which an FS should > call on a bio that the FS wants metadata crypted. The function will add > an encryption context with the metadata encryption key set up by the call > to the above mentioned fscrypt_setup_metadata_encryption(). > > The patch also introduces fscrypt_metadata_crypt_prepare_all_devices(). > Filesystems that use multiple devices should call this function once all > the underlying devices have been determined. An FS might only be able to > determine all the underlying devices after some initial processing that > might already require metadata en/decryption, which is why this function > is separate from fscrypt_setup_metadata_encryption(). > > Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS > will encrypt every block (that's not being encrypted by some other > encryption key, e.g. a per-file key) with the metadata encryption key > except the superblock (and the redundant copy of the superblock). The DUN > of a block is the offset of the block from the start of the F2FS > filesystem. Why not using nid as DUN, then GC could migrate encrypted node block directly via meta inode's address space like we do for encrypted data block, rather than decrypting node block to node page and then encrypting node page with DUN of new blkaddr it migrates to. Thanks, > > Please refer to the commit message for why the superblock was excluded from > en/decryption, and other limitations. The superblock and its copy are > stored in plaintext on disk. The encryption algorithm used for metadata > encryption is stored within the superblock itself. Changes to the userspace > tools (that are required to test out metadata encryption with F2FS) are > also being sent out - I'll post a link as a reply to this mail once it's > out. > > Satya Tangirala (3): > fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device > fscrypt: Add metadata encryption support > f2fs: Add metadata encryption support > > Documentation/filesystems/f2fs.rst | 12 ++ > fs/crypto/Kconfig | 6 + > fs/crypto/Makefile | 1 + > fs/crypto/fscrypt_private.h | 19 +++ > fs/crypto/inline_crypt.c | 37 +---- > fs/crypto/metadata_crypt.c | 220 +++++++++++++++++++++++++++++ > fs/f2fs/data.c | 24 ++-- > fs/f2fs/f2fs.h | 2 + > fs/f2fs/super.c | 83 +++++++++-- > include/linux/f2fs_fs.h | 3 +- > include/linux/fs.h | 3 + > include/linux/fscrypt.h | 51 ++++++- > 12 files changed, 410 insertions(+), 51 deletions(-) > create mode 100644 fs/crypto/metadata_crypt.c >
On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote: > On 2020/10/5 15:36, Satya Tangirala wrote: > > This patch series adds support for metadata encryption to F2FS using > > blk-crypto. > > It looks this implementation is based on hardware crypto engine, could you > please add this info into f2fs.rst as well like inlinecrypt... > To be precise, the implementation requires either a hardware crypto engine *or* blk-crypto-fallback. I tried to clarify this a little better in the commit messages and in fscrypt.rst, but thinking about it again now, I think I should add a section about metadata encryption at the end of f2fs.rst. I'll do that when I send out v3 of this patch series (I just sent out v2). > > > > Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS > > will encrypt every block (that's not being encrypted by some other > > encryption key, e.g. a per-file key) with the metadata encryption key > > except the superblock (and the redundant copy of the superblock). The DUN > > of a block is the offset of the block from the start of the F2FS > > filesystem. > > Why not using nid as DUN, then GC could migrate encrypted node block directly via > meta inode's address space like we do for encrypted data block, rather than > decrypting node block to node page and then encrypting node page with DUN of new > blkaddr it migrates to. > The issue is, the bi_crypt_context in a bio holds a single DUN value, which is the DUN for the first data unit in the bio. blk-crypto assumes that the DUN of each subsequent data unit can be computed by simply incrementing the DUN. So physically contiguous data units can only be put into the same bio if they also have contiguous DUNs. I don't know much about nids, but if the nid is invariant w.r.t the physical block location, then there might be more fragmentation of bios in regular read/writes (because physical contiguity may have no relation to DUN contiguity). So I think we should continue using the fsblk number as the DUN.
On 2020/12/17 23:44, Satya Tangirala wrote: > On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote: >> On 2020/10/5 15:36, Satya Tangirala wrote: >>> This patch series adds support for metadata encryption to F2FS using >>> blk-crypto. >> >> It looks this implementation is based on hardware crypto engine, could you >> please add this info into f2fs.rst as well like inlinecrypt... >> > To be precise, the implementation requires either a hardware crypto > engine *or* blk-crypto-fallback. I tried to clarify this a little better > in the commit messages and in fscrypt.rst, but thinking about it again > now, I think I should add a section about metadata encryption at the end > of f2fs.rst. I'll do that when I send out v3 of this patch series (I > just sent out v2). >>> >>> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS >>> will encrypt every block (that's not being encrypted by some other >>> encryption key, e.g. a per-file key) with the metadata encryption key >>> except the superblock (and the redundant copy of the superblock). The DUN >>> of a block is the offset of the block from the start of the F2FS >>> filesystem. >> >> Why not using nid as DUN, then GC could migrate encrypted node block directly via >> meta inode's address space like we do for encrypted data block, rather than >> decrypting node block to node page and then encrypting node page with DUN of new >> blkaddr it migrates to. >> > The issue is, the bi_crypt_context in a bio holds a single DUN value, > which is the DUN for the first data unit in the bio. blk-crypto assumes > that the DUN of each subsequent data unit can be computed by simply > incrementing the DUN. So physically contiguous data units can only be put > into the same bio if they also have contiguous DUNs. I don't know much > about nids, but if the nid is invariant w.r.t the physical block location, > then there might be more fragmentation of bios in regular read/writes Correct, considering performance of in batch node flush, it will be better to use pba as IV value. But, what's the plan about supporting software encryption for metadata? Current f2fs write flow will handle all operations which may encounter failure before allocating block address for node, if we do allocation first, and then use pba as IV to encrypt node block, it will be a little complicated to revert allocation if we fail to encrypt node block. Thanks, > (because physical contiguity may have no relation to DUN contiguity). So I > think we should continue using the fsblk number as the DUN. > . >
On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote: > On 2020/12/17 23:44, Satya Tangirala wrote: > > On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote: > > > Why not using nid as DUN, then GC could migrate encrypted node block directly via > > > meta inode's address space like we do for encrypted data block, rather than > > > decrypting node block to node page and then encrypting node page with DUN of new > > > blkaddr it migrates to. > > > > > The issue is, the bi_crypt_context in a bio holds a single DUN value, > > which is the DUN for the first data unit in the bio. blk-crypto assumes > > that the DUN of each subsequent data unit can be computed by simply > > incrementing the DUN. So physically contiguous data units can only be put > > into the same bio if they also have contiguous DUNs. I don't know much > > about nids, but if the nid is invariant w.r.t the physical block location, > > then there might be more fragmentation of bios in regular read/writes > > Correct, considering performance of in batch node flush, it will be better to > use pba as IV value. > > But, what's the plan about supporting software encryption for metadata? Current > f2fs write flow will handle all operations which may encounter failure before > allocating block address for node, if we do allocation first, and then use pba > as IV to encrypt node block, it will be a little complicated to revert allocation > if we fail to encrypt node block. > Software encryption for metadata is supported through the blk-crypto framework - so encryption will happen in the block layer, not the filesystem layer. So there's nothing extra/special we need to do if there's an encryption failure - an encryption failure is no different from a read/write failure in a lower layer from f2fs' perspective.
On 2020/12/18 19:53, Satya Tangirala wrote: > On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote: >> On 2020/12/17 23:44, Satya Tangirala wrote: >>> On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote: >>>> Why not using nid as DUN, then GC could migrate encrypted node block directly via >>>> meta inode's address space like we do for encrypted data block, rather than >>>> decrypting node block to node page and then encrypting node page with DUN of new >>>> blkaddr it migrates to. >>>> >>> The issue is, the bi_crypt_context in a bio holds a single DUN value, >>> which is the DUN for the first data unit in the bio. blk-crypto assumes >>> that the DUN of each subsequent data unit can be computed by simply >>> incrementing the DUN. So physically contiguous data units can only be put >>> into the same bio if they also have contiguous DUNs. I don't know much >>> about nids, but if the nid is invariant w.r.t the physical block location, >>> then there might be more fragmentation of bios in regular read/writes >> >> Correct, considering performance of in batch node flush, it will be better to >> use pba as IV value. >> >> But, what's the plan about supporting software encryption for metadata? Current >> f2fs write flow will handle all operations which may encounter failure before >> allocating block address for node, if we do allocation first, and then use pba >> as IV to encrypt node block, it will be a little complicated to revert allocation >> if we fail to encrypt node block. >> > Software encryption for metadata is supported through the blk-crypto blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may be encrypted twice? And why not supporting hardware encryption for metadata in blk-crypto? then both f2fs and ext4 can use inline-encryption based blk-crypto? Thanks, > framework - so encryption will happen in the block layer, not the > filesystem layer. So there's nothing extra/special we need to do if > there's an encryption failure - an encryption failure is no different > from a read/write failure in a lower layer from f2fs' perspective. > . >
On Tue, Dec 22, 2020 at 07:47:45PM +0800, Chao Yu wrote: > On 2020/12/18 19:53, Satya Tangirala wrote: > > On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote: > > > But, what's the plan about supporting software encryption for metadata? Current > > > f2fs write flow will handle all operations which may encounter failure before > > > allocating block address for node, if we do allocation first, and then use pba > > > as IV to encrypt node block, it will be a little complicated to revert allocation > > > if we fail to encrypt node block. > > > > > Software encryption for metadata is supported through the blk-crypto > > blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may > be encrypted twice? blk-crypto will only encrypt bios as directed to do so by the encryption context set on the bio. That encryption context is constructed by the submitter of the bio - in our case, the submitter is the filesystem. So the filesystem decides which bio gets encrypted with which key/algorithm/etc (and in the current implementation, each bio only supports a single bi_crypt_context, so *only one* layer of encryption is possible with blk-crypto anyway). So no, data won't be encrypted twice, because F2FS/fscrypt ensure that it does not (and the filesystem knows exactly which blocks need metadata encryption, and which blocks need FBE). > > And why not supporting hardware encryption for metadata in blk-crypto? then > both f2fs and ext4 can use inline-encryption based blk-crypto? > I may be misunderstanding what you're asking, but I think you're asking why not make blk-crypto do metadata encryption (without explicit involvement from filesystems)? Or more generally, why not do metadata encryption below the filesystem layer? As mentioned above, the filesystem is what knows which blocks need to be metadata encrypted and which blocks need to be FBE encrypted (or even just read without any encryption at all) - the block layer doesn't have this information, and so can't effectively decide which blocks to use the metadata encryption key on. Fwiw, Android does take a somewhat similar approach to what you're suggesting here (I explain more in detail in the cover letter for v2 of this patch series at https://lore.kernel.org/linux-fscrypt/20201217150435.1505269-1-satyat@google.com/ ). In Android, we have a new DM target (called dm-default-key) that adds an encryption context to any bio that doesn't already have an encryption context - so the assumption in general is that if the filesystem wants to use an FBE key, it would have already set the encryption context on the bio, so if a bio reaches dm-default-key without an encryption context, it must mean that it needs metadata encryption. However, that assumption doesn't always hold because F2FS sometimes needs to read the ciphertext of FBE files without having the file's FBE key available - in those situations, F2FS will send a bio without any encryption context, but will also tell dm-default-key to *not* add the metadata encryption context. That's a layering violation, which is why I'm not using that approach here. Does that answer your question? Or am I misunderstanding what you're asking? > Thanks, > > > framework - so encryption will happen in the block layer, not the > > filesystem layer. So there's nothing extra/special we need to do if > > there's an encryption failure - an encryption failure is no different > > from a read/write failure in a lower layer from f2fs' perspective. > > . > >
On 2020/12/24 18:13, Satya Tangirala wrote: > On Tue, Dec 22, 2020 at 07:47:45PM +0800, Chao Yu wrote: >> On 2020/12/18 19:53, Satya Tangirala wrote: >>> On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote: >>>> But, what's the plan about supporting software encryption for metadata? Current >>>> f2fs write flow will handle all operations which may encounter failure before >>>> allocating block address for node, if we do allocation first, and then use pba >>>> as IV to encrypt node block, it will be a little complicated to revert allocation >>>> if we fail to encrypt node block. >>>> >>> Software encryption for metadata is supported through the blk-crypto >> >> blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may >> be encrypted twice? > blk-crypto will only encrypt bios as directed to do so by the encryption > context set on the bio. That encryption context is constructed by the > submitter of the bio - in our case, the submitter is the filesystem. > So the filesystem decides which bio gets encrypted with > which key/algorithm/etc (and in the current implementation, each bio > only supports a single bi_crypt_context, so *only one* layer of > encryption is possible with blk-crypto anyway). So no, data won't be > encrypted twice, because F2FS/fscrypt ensure that it does not (and the > filesystem knows exactly which blocks need metadata encryption, and > which blocks need FBE). Oh, sorry, I misunderstand blk-crypto as dm-crypt... So once hardware encryption is absent, blk-crypto will use blk-crypto-fallback to encrypt bio data with software crypto, I see. >> >> And why not supporting hardware encryption for metadata in blk-crypto? then >> both f2fs and ext4 can use inline-encryption based blk-crypto? >> > I may be misunderstanding what you're asking, but I think you're asking > why not make blk-crypto do metadata encryption (without explicit > involvement from filesystems)? Or more generally, why not do metadata > encryption below the filesystem layer? Yes. > > As mentioned above, the filesystem is what knows which blocks need to be > metadata encrypted and which blocks need to be FBE encrypted (or even > just read without any encryption at all) - the block layer doesn't have > this information, and so can't effectively decide which blocks to use > the metadata encryption key on. Fwiw, Android does take a somewhat > similar approach to what you're suggesting here (I explain more in > detail in the cover letter for v2 of this patch series at > https://lore.kernel.org/linux-fscrypt/20201217150435.1505269-1-satyat@google.com/ Ah, thanks for all your detailed explanation, now, I can see the context. > ). In Android, we have a new DM target (called dm-default-key) that adds > an encryption context to any bio that doesn't already have an encryption > context - so the assumption in general is that if the filesystem wants to > use an FBE key, it would have already set the encryption context on the > bio, so if a bio reaches dm-default-key without an encryption context, > it must mean that it needs metadata encryption. However, that assumption > doesn't always hold because F2FS sometimes needs to read the ciphertext > of FBE files without having the file's FBE key available - in those > situations, F2FS will send a bio without any encryption context, but > will also tell dm-default-key to *not* add the metadata encryption > context. That's a layering violation, which is why I'm not using that > approach here. > > Does that answer your question? Or am I misunderstanding what you're > asking? Yup, thank you! Thanks, >> Thanks, >> >>> framework - so encryption will happen in the block layer, not the >>> filesystem layer. So there's nothing extra/special we need to do if >>> there's an encryption failure - an encryption failure is no different >>> from a read/write failure in a lower layer from f2fs' perspective. >>> . >>> > . >