| Message ID | 20150501001855.GA31516@gondor.apana.org.au |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, May 01, 2015 at 08:18:55AM +0800, Herbert Xu wrote: > This patch adds a tristate EXT4_ENCRYPTION to do the selections > for EXT4_FS_ENCRYPTION because selecting from a bool causes all > the selected options to be built-in, even if EXT4 itself is a > module. Thanks for pointing out the problem! This certainly fixes the problem, but it's a bit confusing; if you answer "y" instead of "m" to the question, it will force the crypto mechanisms to be built-in, and that's not necessarily obvious. That being said, I suspect the people who will care the most about this will be people building the distro kernels, and it will hopefully be obvious to them. In any case, I can't find a better way of doing this, so thanks, applied. I'll get this pushed to Linus ASAP. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, 01.05.2015, 03:18, Herbert Xu kirjoitti: > This patch adds a tristate EXT4_ENCRYPTION to do the selections > for EXT4_FS_ENCRYPTION because selecting from a bool causes all > the selected options to be built-in, even if EXT4 itself is a > module. Hmm, are you sure? Since CONFIG_EXT4_FS_ENCRYPTION itself depends on CONFIG_EXT4_FS, the selector for the selected options becomes (CONFIG_EXT4_FS_ENCRYPTION && CONFIG_EXT4_FS && CONFIG_BLOCK). Per my testing on git master (without this patch), if EXT4_FS=m and EXT4_FS_ENCRYPTION=y, both "built-in" and "module" options are allowed for the selected options (checked CONFIG_ENCRYPTED_KEYS myself). So selector "(A=y && B=m)" results in requirement ">=m", which seems reasonable (otherwise even just CONFIG_BLOCK=y would force them to y). Am I missing something or this patch unneeded? > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig > index 18228c2..024f228 100644 > --- a/fs/ext4/Kconfig > +++ b/fs/ext4/Kconfig > @@ -64,8 +64,8 @@ config EXT4_FS_SECURITY > If you are not using a security module that requires using > extended attributes for file security labels, say N. > > -config EXT4_FS_ENCRYPTION > - bool "Ext4 Encryption" > +config EXT4_ENCRYPTION > + tristate "Ext4 Encryption" > depends on EXT4_FS > select CRYPTO_AES > select CRYPTO_CBC > @@ -81,6 +81,11 @@ config EXT4_FS_ENCRYPTION > efficient since it avoids caching the encrypted and > decrypted pages in the page cache. > > +config EXT4_FS_ENCRYPTION > + bool > + default y > + depends on EXT4_ENCRYPTION > + > config EXT4_DEBUG > bool "EXT4 debugging support" > depends on EXT4_FS >
On Sun, May 03, 2015 at 03:34:14PM +0300, Anssi Hannula wrote: > Hi, > > 01.05.2015, 03:18, Herbert Xu kirjoitti: > > This patch adds a tristate EXT4_ENCRYPTION to do the selections > > for EXT4_FS_ENCRYPTION because selecting from a bool causes all > > the selected options to be built-in, even if EXT4 itself is a > > module. > > Hmm, are you sure? > > Since CONFIG_EXT4_FS_ENCRYPTION itself depends on CONFIG_EXT4_FS, the > selector for the selected options becomes (CONFIG_EXT4_FS_ENCRYPTION && > CONFIG_EXT4_FS && CONFIG_BLOCK). > > Per my testing on git master (without this patch), if EXT4_FS=m and > EXT4_FS_ENCRYPTION=y, both "built-in" and "module" options are allowed > for the selected options (checked CONFIG_ENCRYPTED_KEYS myself). I believe the situation which is causing concern is when someone wants to build a kernel where EXT4_FS=y, but they want the cryptographic algorithms to be modules. In that case, since EXT4_FS_ENCRYPTION is 'y', it forces the all of the crypto modules to be built into the kernel, and so it forecloses that option from someone who is building or packaging a kernel. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
03.05.2015, 20:53, Theodore Ts'o kirjoitti: > On Sun, May 03, 2015 at 03:34:14PM +0300, Anssi Hannula wrote: >> Hi, >> >> 01.05.2015, 03:18, Herbert Xu kirjoitti: >>> This patch adds a tristate EXT4_ENCRYPTION to do the selections >>> for EXT4_FS_ENCRYPTION because selecting from a bool causes all >>> the selected options to be built-in, even if EXT4 itself is a >>> module. >> >> Hmm, are you sure? >> >> Since CONFIG_EXT4_FS_ENCRYPTION itself depends on CONFIG_EXT4_FS, the >> selector for the selected options becomes (CONFIG_EXT4_FS_ENCRYPTION && >> CONFIG_EXT4_FS && CONFIG_BLOCK). >> >> Per my testing on git master (without this patch), if EXT4_FS=m and >> EXT4_FS_ENCRYPTION=y, both "built-in" and "module" options are allowed >> for the selected options (checked CONFIG_ENCRYPTED_KEYS myself). > > I believe the situation which is causing concern is when someone wants > to build a kernel where EXT4_FS=y, but they want the cryptographic > algorithms to be modules. In that case, since EXT4_FS_ENCRYPTION is > 'y', it forces the all of the crypto modules to be built into the > kernel, and so it forecloses that option from someone who is building > or packaging a kernel. Ah, OK, so not "EXT4 itself as a module" like the commit message said :) For the situation you described I don't see a better solution either.
On Sun, May 03, 2015 at 09:29:02PM +0300, Anssi Hannula wrote: > > I believe the situation which is causing concern is when someone wants > > to build a kernel where EXT4_FS=y, but they want the cryptographic > > algorithms to be modules. In that case, since EXT4_FS_ENCRYPTION is > > 'y', it forces the all of the crypto modules to be built into the > > kernel, and so it forecloses that option from someone who is building > > or packaging a kernel. > > Ah, OK, so not "EXT4 itself as a module" like the commit message said :) > > For the situation you described I don't see a better solution either. Thanks for pointing out problem in the commit message. I guess I wasn't reading all that carefully, but started experimenting, and came up with some case where, if they aren't lack _bugs_, do constrain flexibility a little. You are correct that various crypto modules can still be built as modules even if ext4 is a module and EXT4_FS_ENCRYPTION is 'y'. The main issue that I was able to find is that if ext4 is _not_ a module, then it also forces the crypto modules to also be built in. (Personally from a performance perspective, I'd always want to make the common crypto modules always built in to avoid pressure on the TLB cache, but I understand that distributions seem to like to build _everything_ as modules (which is one of the reasons why I generally don't use distro kernels myself. :-) In any case, I'll correct the commit message so that it describes the problem which it addresses more clearly. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 03, 2015 at 05:11:18PM -0400, Theodore Ts'o wrote: > > Thanks for pointing out problem in the commit message. I guess I > wasn't reading all that carefully, but started experimenting, and came > up with some case where, if they aren't lack _bugs_, do constrain > flexibility a little. You are correct that various crypto modules can > still be built as modules even if ext4 is a module and > EXT4_FS_ENCRYPTION is 'y'. The main issue that I was able to find is > that if ext4 is _not_ a module, then it also forces the crypto modules > to also be built in. Sorry for the confusion. Please just revert my patch. I was trying to figure out what was causing various crypto modules to be built-in in my test config and this was the latest change that *looked* as if it might have caused my problem. Obviously I didn't test it properly after making that change. It should just be reverted because if ext4 was built-in then you do want to have the crypto stuff built-in just in case the root fs was encrypted. Cheers,
On Mon, May 04, 2015 at 09:00:16AM +0800, Herbert Xu wrote: > > Sorry for the confusion. Please just revert my patch. > > I was trying to figure out what was causing various crypto modules > to be built-in in my test config and this was the latest change that > *looked* as if it might have caused my problem. Obviously I didn't > test it properly after making that change. > > It should just be reverted because if ext4 was built-in then you > do want to have the crypto stuff built-in just in case the root fs > was encrypted. Whoops, I _just_ sent a pull request to Linus. The patch as it stands actually allows both behaviors, depending on whether you answer 'y' or 'm' to EXT4_ENCRYPTION question. Given that one of the main purposes of per-filesystem encryption is that we only have to encrypt the user files, so the system files can remain unencrypted for performance reasons[1], I can imagine scenarios where it's conceivable that someone might want to keep the crypto as modules. My main unhappiness with the Kconfig option is that it's a bit user unfriendly/confusing what it means for CONFIG_EXT4_ENCRYPTION to be 'y' versus 'm'. So I may end up reverting it, but since I've already sent the pull request to Linus, I'm going to sleep on this for a bit. Cheers, - Ted [1] Though not for Intel chips; Intel acceleration of AES is so fast there you might as well encrypt everything; and for single-user laptops I still recommend dm-crypt. Unfortunately, there are some ARM chips which either do not have hardware accelerated AES, or where their hardware accleration is decidedly poor.... -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig index 18228c2..024f228 100644 --- a/fs/ext4/Kconfig +++ b/fs/ext4/Kconfig @@ -64,8 +64,8 @@ config EXT4_FS_SECURITY If you are not using a security module that requires using extended attributes for file security labels, say N. -config EXT4_FS_ENCRYPTION - bool "Ext4 Encryption" +config EXT4_ENCRYPTION + tristate "Ext4 Encryption" depends on EXT4_FS select CRYPTO_AES select CRYPTO_CBC @@ -81,6 +81,11 @@ config EXT4_FS_ENCRYPTION efficient since it avoids caching the encrypted and decrypted pages in the page cache. +config EXT4_FS_ENCRYPTION + bool + default y + depends on EXT4_ENCRYPTION + config EXT4_DEBUG bool "EXT4 debugging support" depends on EXT4_FS
This patch adds a tristate EXT4_ENCRYPTION to do the selections for EXT4_FS_ENCRYPTION because selecting from a bool causes all the selected options to be built-in, even if EXT4 itself is a module. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>