On Fri, Jul 18, 2025 at 08:37:04AM -0700, Bart Van Assche wrote: > On 7/16/25 9:43 PM, Christoph Hellwig wrote: > > Getting back to this. While the ton is a bit snarky, it brings up a good > > point. Relying on the block layer to ensure that data is always > > encrypted seems like a bad idea, given that is really not what the block > > layer is about, and definitively not high on the mind of anyone touching > > the code. So I would not want to rely on the block layer developers to > > ensure that data is encrypted properly through APIs not one believes part > > that mission. > > > > So I think you'd indeed be much better off not handling the (non-inline) > > incryption in the block layer. > > > > Doing that in fact sounds pretty easy - instead of calling the > > blk-crypto-fallback code from inside the block layer, call it from the > > callers instead of submit_bio when inline encryption is not actually > > supported, e.g. > > > > if (!blk_crypto_config_supported(bdev, &crypto_cfg)) > > blk_crypto_fallback_submit_bio(bio); > > else > > submit_bio(bio); > > > > combined with checks in the low-level block code that we never get a > > crypto context into the low-level submission into ->submit_bio or > > ->queue_rq when not supported. > > > > That approach not only is much easier to verify for correct encryption > > operation, but also makes things like bio splitting and the required > > memory allocation for it less fragile. > > Has it ever been considered to merge the inline encryption code into > dm-crypt or convert the inline encryption fallback code into a new dm > driver? If user space code would insert a dm device between filesystems > and block devices if hardware encryption is not supported that would > have the following advantages: > * No filesystem implementations would have to be modified. > * It would make it easier to deal with bio splitting since dm drivers > can set stacking limits in their .io_hints() callback. Yes, this was considered back when blk-crypto-fallback was being added. But it is useful to support blk-crypto unconditionally without userspace having to set up a dm device: - It allows testing the inline encryption code paths in ext4 and f2fs with xfstests on any system, just by adding the inlinecrypt mount option. See e.g. https://lore.kernel.org/linux-block/20191031205045.GG16197@xxxxxxx/ and https://lore.kernel.org/linux-block/20200515170059.GA1009@sol.localdomain/ - It allows upper layers to just use blk-crypto instead of having both blk-crypto and non-blk-crypto code paths. While I've been late on converting fscrypt to actually rely on this, it still might be a good idea, especially as we now need to revisit the code for reasons like large folio support. (And Christoph seems to support this too -- see https://lore.kernel.org/linux-block/20250715062112.GC18672@xxxxxx/) But, as suggested at https://lore.kernel.org/linux-block/20250717044342.GA26995@xxxxxx/ it should also be okay to reorganize things so that the regular submit_bio() does not support the fallback, and upper layers have to call a different function blk_crypto_fallback_submit_bio() if they want the fallback. I don't think that would help with the splitting issue directly, but perhaps we could make the filesystems just not submit bios that would need splitting by blk-crypto-fallback, which would solve the issue. - Eric
