From: Christoph Hellwig <hch@infradead.org> To: Eric Biggers <ebiggers@kernel.org>, Satya Tangirala <satyat@google.com>, linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>, Kuohong Wang <kuohong.wang@mediatek.com>, Barani Muthukumaran <bmuthuku@qti.qualcomm.com>, linux-f2fs-devel@lists.sourceforge.net, linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v5 7/9] fscrypt: add inline encryption support Date: Thu, 31 Oct 2019 14:21:03 -0700 [thread overview] Message-ID: <20191031212103.GA6244@infradead.org> (raw) In-Reply-To: <20191031202125.GA111219@gmail.com> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@infradead.org> To: Eric Biggers <ebiggers@kernel.org>, Satya Tangirala <satyat@google.com>, linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>, Kuohong Wang <kuohong.wang@mediatek.com>, Barani Muthukumaran <bmuthuku@qti.qualcomm.com>, linux-f2fs-devel@lists.sourceforge.net, linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [f2fs-dev] [PATCH v5 7/9] fscrypt: add inline encryption support Date: Thu, 31 Oct 2019 14:21:03 -0700 [thread overview] Message-ID: <20191031212103.GA6244@infradead.org> (raw) In-Reply-To: <20191031202125.GA111219@gmail.com> 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
next prev parent reply other threads:[~2019-10-31 21:21 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-28 7:20 [PATCH v5 0/9] Inline Encryption Support Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-28 7:20 ` [PATCH v5 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:04 ` Christoph Hellwig 2019-10-31 18:04 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 2/9] block: Add encryption context to struct bio Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:16 ` Christoph Hellwig 2019-10-31 18:16 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 3/9] block: blk-crypto for Inline Encryption Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 17:57 ` Christoph Hellwig 2019-10-31 17:57 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 20:50 ` Theodore Y. Ts'o 2019-10-31 20:50 ` [f2fs-dev] " Theodore Y. Ts'o 2019-10-31 21:22 ` Christoph Hellwig 2019-10-31 21:22 ` [f2fs-dev] " Christoph Hellwig 2019-11-05 2:01 ` Eric Biggers 2019-11-05 2:01 ` [f2fs-dev] " Eric Biggers 2019-11-05 15:39 ` Christoph Hellwig 2019-11-05 15:39 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 4/9] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-28 7:20 ` [PATCH v5 5/9] scsi: ufs: UFS crypto API Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:23 ` Christoph Hellwig 2019-10-31 18:23 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:26 ` Christoph Hellwig 2019-10-31 18:26 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 7/9] fscrypt: add inline encryption support Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:32 ` Christoph Hellwig 2019-10-31 18:32 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 20:21 ` Eric Biggers 2019-10-31 20:21 ` [f2fs-dev] " Eric Biggers 2019-10-31 21:21 ` Christoph Hellwig [this message] 2019-10-31 21:21 ` Christoph Hellwig 2019-10-31 22:25 ` Eric Biggers 2019-10-31 22:25 ` [f2fs-dev] " Eric Biggers 2019-11-05 0:15 ` Christoph Hellwig 2019-11-05 0:15 ` [f2fs-dev] " Christoph Hellwig 2019-11-05 1:03 ` Eric Biggers 2019-11-05 1:03 ` [f2fs-dev] " Eric Biggers 2019-11-05 3:12 ` Eric Biggers 2019-11-05 3:12 ` [f2fs-dev] " Eric Biggers 2019-10-28 7:20 ` [PATCH v5 8/9] f2fs: " Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 17:14 ` Jaegeuk Kim 2019-10-31 17:14 ` [f2fs-dev] " Jaegeuk Kim 2019-10-28 7:20 ` [PATCH v5 9/9] ext4: " Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191031212103.GA6244@infradead.org \ --to=hch@infradead.org \ --cc=bmuthuku@qti.qualcomm.com \ --cc=boojin.kim@samsung.com \ --cc=ebiggers@kernel.org \ --cc=kuohong.wang@mediatek.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fscrypt@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=satyat@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.