Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
On Thu, Oct 31, 2019 at 02:21:03PM -0700, Christoph Hellwig wrote: > > > > > > Btw, I'm not happy about the 8-byte IV assumptions everywhere here. > > > That really should be a parameter, not hardcoded. > > > > To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, > > but > > rather in what the blk-crypto API provides. If blk-crypto were to provide > > longer IV support, fs/crypto/ would pretty easily be able to make use of it. > > That's what I meant - we hardcode the value in fscrypt. Instead we need > to expose the size from blk-crypt and check for it. > > > > > (And if IVs >= 24 bytes were supported and we added AES-128-CBC-ESSIV and > > Adiantum support to blk-crypto.c, then inline encryption would be able to do > > everything that the existing filesystem-layer contents encryption can do.) > > > > Do you have anything in mind for how to make the API support longer IVs in a > > clean way? Are you thinking of something like: > > > > #define BLK_CRYPTO_MAX_DUN_SIZE 24 > > > > u8 dun[BLK_CRYPTO_MAX_DUN_SIZE]; > > int dun_size; > > > > We do have to perform arithmetic operations on it, so a byte array would > > make it > > ugly and slower, but it should be possible... > > Well, we could make it an array of u64s, which means we can do all the > useful arithmetics on components on one of them. But I see the point, > this adds significant complexity for no real short term gain, and we > should probably postponed it until needed. Maybe just document the > assumptions a little better. Just in case it's not obvious to anyone, I should also mention that being limited to specifying a 64-bit DUN doesn't prevent hardware that accepts a longer IV (e.g. 128 bits) from being used. It would just be a matter of zero-padding the IV in the driver rather than in hardware. The actual limitation we're talking about here is in the range of IVs that can be specified. A 64-bit DUN only allows the first 64 bits of the IV to be nonzero. That works for fscrypt in all cases except DIRECT_KEY policies, and it would work for dm-crypt using the usual dm-crypt IV generator (plain64). But for inline encryption to be compatible with DIRECT_KEY fscrypt policies or with certain other dm-crypt IV generators, we'd need the ability to specify more IV bits. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
On Mon, Nov 04, 2019 at 04:15:54PM -0800, Christoph Hellwig wrote: > > 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. fs/crypto/ uses ->has_stable_inodes() and ->get_ino_and_lblk_bits() to print an appropriate error message. If we changed it to a simple flag we'd have to print a less useful error message. Also, people are basically guaranteed to not understand what "SB_IV_INO_LBLK_64_SUPPORT" means exactly, and are likely to copy-and-paste it incorrectly when adding fscrypt support to a new filesystem. Also it would make it more difficult to add other fscrypt IV generation schemes in the future as we'd then need to add another sb flag (e.g. SB_IV_INO_LBLK_128) and make filesystem-specific changes, rather than change fs/crypto/ only. So personally I'd prefer to keep ->has_stable_inodes() and ->get_ino_and_lblk_bits() for now. Replacing ->inline_crypt_enabled() with SB_INLINE_CRYPT makes much more sense though. - Eric ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
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.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
On Thu, Oct 31, 2019 at 02:21:03PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 31, 2019 at 01:21:26PM -0700, Eric Biggers wrote:
> > > > + /* The file must need contents encryption, not filenames
> > > > encryption */
> > > > + if (!S_ISREG(inode->i_mode))
> > > > + return false;
> > >
> > > But that isn't really what the check checks for..
> >
> > This is how fscrypt has always worked. The type of encryption to do is
> > determined as follows:
> >
> > S_ISREG() => contents encryption
> > S_ISDIR() || S_ISLNK() => filenames encryption
> >
> > See e.g. select_encryption_mode(), and similar checks elsewhere in
> > fs/{crypto,ext4,f2fs}/.
> >
> > Do you have a suggestion to make it clearer?
>
> Maybe have a fscrypt_content_encryption helper that currently
> evaluates to S_ISREG(inode->i_mode) as that documents the intent?
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);
}
It could be used here too, though only S_ISREG() is actually needed here, so it
wouldn't be as good of a fit.
Anyway, this will need to be a separate cleanup.
>
> > > > + /* The filesystem must be mounted with -o inlinecrypt */
> > > > + if (!sb->s_cop->inline_crypt_enabled ||
> > > > + !sb->s_cop->inline_crypt_enabled(sb))
> > > > + return false;
> > >
> > > So please add a SB_* flag for that option instead of the weird
> > > indirection.
> >
> > IMO it's not really "weird" given that the point of the fscrypt_operations
> > is to
> > expose filesystem-specific stuff to fs/crypto/. But yes, using one of the
> > SB_*
> > bits would make it simpler, so if people are fine with that we'll do that.
>
> I think that is much simpler. Additionally it could also replace the
> need for the has_stable_inodes and get_ino_and_lblk_bits methods, which
> are pretty weird. Instead just document the requirements for the
> SB_INLINE_CRYPT flag and handle the rest in the file system. That is
> for f2f always set it, and for ext4 set it based on s_inodes_count.
> Which brings me to:
>
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. 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.
> > >
> > > Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> > > That really should be a parameter, not hardcoded.
> >
> > To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/,
> > but
> > rather in what the blk-crypto API provides. If blk-crypto were to provide
> > longer IV support, fs/crypto/ would pretty easily be able to make use of it.
>
> That's what I meant - we hardcode the value in fscrypt. Instead we need
> to expose the size from blk-crypt and check for it.
>
If we add variable-length IV support, I think it should be handled in the same
way as crypto algorithms. The current model is that the bio submitter doesn't
need to check whether the crypto algorithm is supported by the device, because
blk-crypto will fall back to the crypto API if needed. Unsupported IV lengths
could be handled in the same way.
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
On Thu, Oct 31, 2019 at 01:21:26PM -0700, Eric Biggers wrote:
> > > + /* The file must need contents encryption, not filenames encryption */
> > > + if (!S_ISREG(inode->i_mode))
> > > + return false;
> >
> > But that isn't really what the check checks for..
>
> This is how fscrypt has always worked. The type of encryption to do is
> determined as follows:
>
> S_ISREG() => contents encryption
> S_ISDIR() || S_ISLNK() => filenames encryption
>
> See e.g. select_encryption_mode(), and similar checks elsewhere in
> fs/{crypto,ext4,f2fs}/.
>
> Do you have a suggestion to make it clearer?
Maybe have a fscrypt_content_encryption helper that currently
evaluates to S_ISREG(inode->i_mode) as that documents the intent?
> > > + /* The filesystem must be mounted with -o inlinecrypt */
> > > + if (!sb->s_cop->inline_crypt_enabled ||
> > > + !sb->s_cop->inline_crypt_enabled(sb))
> > > + return false;
> >
> > So please add a SB_* flag for that option instead of the weird
> > indirection.
>
> IMO it's not really "weird" given that the point of the fscrypt_operations is
> to
> expose filesystem-specific stuff to fs/crypto/. But yes, using one of the
> SB_*
> bits would make it simpler, so if people are fine with that we'll do that.
I think that is much simpler. Additionally it could also replace the
need for the has_stable_inodes and get_ino_and_lblk_bits methods, which
are pretty weird. Instead just document the requirements for the
SB_INLINE_CRYPT flag and handle the rest in the file system. That is
for f2f always set it, and for ext4 set it based on s_inodes_count.
Which brings me to:
> >
> > Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> > That really should be a parameter, not hardcoded.
>
> To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, but
> rather in what the blk-crypto API provides. If blk-crypto were to provide
> longer IV support, fs/crypto/ would pretty easily be able to make use of it.
That's what I meant - we hardcode the value in fscrypt. Instead we need
to expose the size from blk-crypt and check for it.
>
> (And if IVs >= 24 bytes were supported and we added AES-128-CBC-ESSIV and
> Adiantum support to blk-crypto.c, then inline encryption would be able to do
> everything that the existing filesystem-layer contents encryption can do.)
>
> Do you have anything in mind for how to make the API support longer IVs in a
> clean way? Are you thinking of something like:
>
> #define BLK_CRYPTO_MAX_DUN_SIZE 24
>
> u8 dun[BLK_CRYPTO_MAX_DUN_SIZE];
> int dun_size;
>
> We do have to perform arithmetic operations on it, so a byte array would make
> it
> ugly and slower, but it should be possible...
Well, we could make it an array of u64s, which means we can do all the
useful arithmetics on components on one of them. But I see the point,
this adds significant complexity for no real short term gain, and we
should probably postponed it until needed. Maybe just document the
assumptions a little better.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
Hi Christoph, thanks for reviewing this.
On Thu, Oct 31, 2019 at 11:32:17AM -0700, Christoph Hellwig wrote:
> > diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> > index 1f4b8a277060..956798debf71 100644
> > --- a/fs/crypto/bio.c
> > +++ b/fs/crypto/bio.c
> > @@ -46,26 +46,38 @@ int fscrypt_zeroout_range(const struct inode *inode,
> > pgoff_t lblk,
> > {
> > const unsigned int blockbits = inode->i_blkbits;
> > const unsigned int blocksize = 1 << blockbits;
> > + const bool inlinecrypt = fscrypt_inode_uses_inline_crypto(inode);
> > struct page *ciphertext_page;
> > struct bio *bio;
> > int ret, err = 0;
> >
> > - ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> > - if (!ciphertext_page)
> > - return -ENOMEM;
> > + if (inlinecrypt) {
> > + ciphertext_page = ZERO_PAGE(0);
> > + } else {
> > + ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> > + if (!ciphertext_page)
> > + return -ENOMEM;
>
> I think you just want to split this into two functions for the
> inline crypto vs not cases.
>
> > @@ -391,6 +450,16 @@ struct fscrypt_master_key {
> > */
> > struct crypto_skcipher *mk_iv_ino_lblk_64_tfms[__FSCRYPT_MODE_MAX + 1];
> >
> > +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> > + /* Raw keys for IV_INO_LBLK_64 policies, allocated on-demand */
> > + u8 *mk_iv_ino_lblk_64_raw_keys[__FSCRYPT_MODE_MAX
> > + 1];
> > +
> > + /* The data unit size being used for inline encryption */
> > + unsigned intmk_data_unit_size;
> > +
> > + /* The filesystem's block device */
> > + struct block_device *mk_bdev;
>
> File systems (including f2fs) can have multiple underlying block
> devices.
Yes, this is a bug: it causes the key to be evicted from only one device.
I'm thinking this might point to deeper problems with the inline encryption API
exposed to filesystems being too difficult to use correctly. We might want to
move to something like:
struct blk_crypto_key_handle *
blk_crypto_prepare_key(struct request_queue *q, const u8 *raw_key,
enum blk_crypto_mode_num crypto_mode, unsigned int
data_unit_size);
int bio_crypt_set_ctx(struct bio *bio, struct blk_crypto_key_handle *h,
u64 dun, gfp_t gfp_mask);
void blk_crypto_destroy_key(struct blk_crypto_key_handle *h);
Each handle would associate a raw_key, mode, and data_unit_size with a specific
keyslot_manager, which would allocate/evict a keyslot as needed for I/O.
blk_crypto_destroy_key() would both evict the keyslot and free the key.
For each key, multi-device filesystems would then need to allocate a handle for
each device. This would force them to handle key eviction correctly.
Satya, any thoughts on this?
>
> > +{
> > + const struct inode *inode = ci->ci_inode;
> > + struct super_block *sb = inode->i_sb;
> > +
> > + /* The file must need contents encryption, not filenames encryption */
> > + if (!S_ISREG(inode->i_mode))
> > + return false;
>
> But that isn't really what the check checks for..
This is how fscrypt has always worked. The type of encryption to do is
determined as follows:
S_ISREG() => contents encryption
S_ISDIR() || S_ISLNK() => filenames encryption
See e.g. select_encryption_mode(), and similar checks elsewhere in
fs/{crypto,ext4,f2fs}/.
Do you have a suggestion to make it clearer?
>
> > + /* The filesystem must be mounted with -o inlinecrypt */
> > + if (!sb->s_cop->inline_crypt_enabled ||
> > + !sb->s_cop->inline_crypt_enabled(sb))
> > + return false;
>
> So please add a SB_* flag for that option instead of the weird
> indirection.
IMO it's not really "weird" given that the point of the fscrypt_operations is to
expose filesystem-specific stuff to fs/crypto/. But yes, using one of the SB_*
bits would make it simpler, so if people are fine with that we'll do that.
>
> > +/**
> > + * fscrypt_inode_uses_inline_crypto - test whether an inode uses inline
> > encryption
>
> This adds an overly long line. This happens many more times in the
> patch.
checkpatch reports 4 overly long lines in the patch, 3 of which are the first
line of kerneldoc comments. For some reason I thought those are recommended to
be one line even if it exceeds 80 characters, but maybe I'm mistaken.
>
> Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
> That really should be a parameter, not hardcoded.
To be clear, the 8-byte IV assumption doesn't really come from fs/crypto/, but
rather in what the blk-crypto API provides. If blk-crypto were to provide
longer IV support, fs/crypto/ would pretty easily be able to make use of it.
(And if IVs >= 24 bytes were supported and we added AES-128-CBC-ESSIV and
Adiantum support to blk-crypto.c, then inline encryption would be able to do
everything that the existing filesystem-layer contents encryption can do.)
Do you have anything in mind for how
Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support
> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> index 1f4b8a277060..956798debf71 100644
> --- a/fs/crypto/bio.c
> +++ b/fs/crypto/bio.c
> @@ -46,26 +46,38 @@ int fscrypt_zeroout_range(const struct inode *inode,
> pgoff_t lblk,
> {
> const unsigned int blockbits = inode->i_blkbits;
> const unsigned int blocksize = 1 << blockbits;
> + const bool inlinecrypt = fscrypt_inode_uses_inline_crypto(inode);
> struct page *ciphertext_page;
> struct bio *bio;
> int ret, err = 0;
>
> - ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> - if (!ciphertext_page)
> - return -ENOMEM;
> + if (inlinecrypt) {
> + ciphertext_page = ZERO_PAGE(0);
> + } else {
> + ciphertext_page = fscrypt_alloc_bounce_page(GFP_NOWAIT);
> + if (!ciphertext_page)
> + return -ENOMEM;
I think you just want to split this into two functions for the
inline crypto vs not cases.
> @@ -391,6 +450,16 @@ struct fscrypt_master_key {
>*/
> struct crypto_skcipher *mk_iv_ino_lblk_64_tfms[__FSCRYPT_MODE_MAX + 1];
>
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> + /* Raw keys for IV_INO_LBLK_64 policies, allocated on-demand */
> + u8 *mk_iv_ino_lblk_64_raw_keys[__FSCRYPT_MODE_MAX
> + 1];
> +
> + /* The data unit size being used for inline encryption */
> + unsigned intmk_data_unit_size;
> +
> + /* The filesystem's block device */
> + struct block_device *mk_bdev;
File systems (including f2fs) can have multiple underlying block
devices.
> +{
> + const struct inode *inode = ci->ci_inode;
> + struct super_block *sb = inode->i_sb;
> +
> + /* The file must need contents encryption, not filenames encryption */
> + if (!S_ISREG(inode->i_mode))
> + return false;
But that isn't really what the check checks for..
> + /* The filesystem must be mounted with -o inlinecrypt */
> + if (!sb->s_cop->inline_crypt_enabled ||
> + !sb->s_cop->inline_crypt_enabled(sb))
> + return false;
So please add a SB_* flag for that option instead of the weird
indirection.
> +/**
> + * fscrypt_inode_uses_inline_crypto - test whether an inode uses inline
> encryption
This adds an overly long line. This happens many more times in the
patch.
Btw, I'm not happy about the 8-byte IV assumptions everywhere here.
That really should be a parameter, not hardcoded.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
