| Message ID | 20221110141225.2308856-1-ndevos@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fs: provide per-filesystem options to disable fscrypt | expand |
On Thu, Nov 10, 2022 at 03:12:21PM +0100, Niels de Vos wrote: > While more filesystems are getting support for fscrypt, it is useful to > be able to disable fscrypt for a selection of filesystems, while > enabling it for others. Could you say why you find it useful? Is it because you are concerned about the increased binary size of a particular file system if fscrypt is enabled? That hasn't been my experience, the hooks to call into fscrypt are small and don't add too much to any one particular file system; the bulk of the code is in fs/crypto. Is it because people are pushing buggy code that doesn't compile if you enable, say, CONFIG_FS_XXX and CONFIG_FSCRYPT at the same time? Is it because a particular distribution doesn't want to support fscrypt with a particular file system? If so, there have been plenty of file system features for say, ext4, xfs, and btrfs, which aren't supported by a distro, but there isn't a CONFIG_FS_XXX_YYY to disable that feature, nor have any distros requested such a thing --- which is good because it would be an explosion of new CONFIG parameters. Or is it something else? Note that nearly all of the file systems will only enable fscrypt if some file system feature flag enabls it. So I'm not sure what's the motivation behind adding this configuration option. If memory serves, early in the fscrypt development we did have per-file system CONFIG's for fscrypt, but we consciously removed it, just as we no longer have per-file system CONFIG's to enable or disable Posix ACL's or extended attributes, in the name of simplifying the kernel config. Cheers, - Ted
On Thu, Nov 10, 2022 at 10:38:37AM -0500, Theodore Ts'o wrote: > On Thu, Nov 10, 2022 at 03:12:21PM +0100, Niels de Vos wrote: > > While more filesystems are getting support for fscrypt, it is useful to > > be able to disable fscrypt for a selection of filesystems, while > > enabling it for others. > > Could you say why you find it useful? Is it because you are concerned > about the increased binary size of a particular file system if fscrypt > is enabled? That hasn't been my experience, the hooks to call into > fscrypt are small and don't add too much to any one particular file > system; the bulk of the code is in fs/crypto. No, this isn't really a concern. I don't think the small size difference is worth the effort. > Is it because people are pushing buggy code that doesn't compile if > you enable, say, CONFIG_FS_XXX and CONFIG_FSCRYPT at the same time? It is a little of this. Not necessarily for the compiling part, more of a functional thing. And that is what comes next... > Is it because a particular distribution doesn't want to support > fscrypt with a particular file system? If so, there have been plenty > of file system features for say, ext4, xfs, and btrfs, which aren't > supported by a distro, but there isn't a CONFIG_FS_XXX_YYY to disable > that feature, nor have any distros requested such a thing --- which is > good because it would be an explosion of new CONFIG parameters. This is mostly why I sent this RFC. We are interested in enabling fscrypt for CephFS (soonish) as a network filesystem, but not for local filesystems (we recommend dm-crypt for those). The idea is that functionality that isn't available, can also not (easily) cause breakage. And, there actually are options like CONFIG_EXT4_FS_POSIX_ACL and CONFIG_EXT4_FS_SECURITY. Because these exist already, I did not expect too much concerns with proposing a CONFIG_EXT4_FS_ENCRYPTION... > Or is it something else? > > Note that nearly all of the file systems will only enable fscrypt if > some file system feature flag enabls it. So I'm not sure what's the > motivation behind adding this configuration option. If memory serves, > early in the fscrypt development we did have per-file system CONFIG's > for fscrypt, but we consciously removed it, just as we no longer have > per-file system CONFIG's to enable or disable Posix ACL's or extended > attributes, in the name of simplifying the kernel config. Thanks for adding some history about this. I understand that extra options are needed while creating/tuning the filesystems. Preventing users from setting the right options in a filesystem is not easy, even if tools from a distribution do not offer setting the options. Disks can be portable, or network-attached, and have options enabled that an other distributions kernel does not (want to) support. Note that even with the additional options, enabling only CONFIG_FS_ENCRYPTION causes all the filesystems that support fscrypt to have it enabled. For users there is no change, except that they now have an option to disable fscrypt support per filesystem. I hope this explains the intention a bit better. And as there are existing options for many filesystems to select support for xattrs, security or ACLs, I hope new options for (de)selecting filesystem encryption support is not too controversial. Thanks! Niels
On Thu, Nov 10, 2022 at 05:47:10PM +0100, Niels de Vos wrote: > And, there actually are options like CONFIG_EXT4_FS_POSIX_ACL and > CONFIG_EXT4_FS_SECURITY. Because these exist already, I did not expect > too much concerns with proposing a CONFIG_EXT4_FS_ENCRYPTION... Actually, I was thinking of getting rid of them, as we've already gotten rid of EXT4_FS_POSIX_ACL.... > Thanks for adding some history about this. I understand that extra > options are needed while creating/tuning the filesystems. Preventing > users from setting the right options in a filesystem is not easy, even > if tools from a distribution do not offer setting the options. Disks can > be portable, or network-attached, and have options enabled that an other > distributions kernel does not (want to) support. Sure, but as I said, there are **tons** of file system features that have not and/or still are not supported for distros, but for which we don't have kernel config knobs. This includes ext4's bigalloc and inline data, btrfs's dedup and reflink support, xfs online fsck, etc., etc., etc. Heck, ext4 is only supported up to a certain size by Red Hat, and we don't have a Kernel config so that the kernel will absolutely refuse to mount an ext4 file system larger than The Officially Supported RHEL Capacity Limit for Ext4. So what makes fscrypt different from all of these other unsupported file system features? There are plenty of times when I've had to explain to customers why, sure they could build their own kernels for RHEL 4 (back in the day when I worked for Big Blue and had to talk to lots of enterprise customers), but if they did, Red Hat support would refuse to give them the time of day if they called asking for help. We didn't set up use digitally signed kernels with trusted boot so that a IBM server would refuse to boot anything other than An Officially Signed RHEL Kernel... What makes fscrypt different that we think we need to enforce this using technical means, other than a simple, "this feature is not supported"? - Ted
On Thu, Nov 10, 2022 at 01:15:38PM -0500, Theodore Ts'o wrote: > On Thu, Nov 10, 2022 at 05:47:10PM +0100, Niels de Vos wrote: > > And, there actually are options like CONFIG_EXT4_FS_POSIX_ACL and > > CONFIG_EXT4_FS_SECURITY. Because these exist already, I did not expect > > too much concerns with proposing a CONFIG_EXT4_FS_ENCRYPTION... > > Actually, I was thinking of getting rid of them, as we've already > gotten rid of EXT4_FS_POSIX_ACL.... Sorry, I meant to say that we had gotten rid of EXT4_FS_XATTR. - Ted
On Thu, Nov 10, 2022 at 05:47:10PM +0100, Niels de Vos wrote: > And, there actually are options like CONFIG_EXT4_FS_POSIX_ACL and > CONFIG_EXT4_FS_SECURITY. Because these exist already, I did not expect > too much concerns with proposing a CONFIG_EXT4_FS_ENCRYPTION... ext4 is a little weird there as most file systems don't do that. So I think these should go away for ext4 as well. > Note that even with the additional options, enabling only > CONFIG_FS_ENCRYPTION causes all the filesystems that support fscrypt to > have it enabled. For users there is no change, except that they now have > an option to disable fscrypt support per filesystem. But why would you do that anyay?
On Thu, Nov 10, 2022 at 03:12:21PM +0100, Niels de Vos wrote: > While more filesystems are getting support for fscrypt, it is useful to > be able to disable fscrypt for a selection of filesystems, while > enabling it for others. > > The new USE_FS_ENCRYPTION define gets picked up in > include/linux/fscrypt.h. This allows filesystems to choose to use the > empty function definitions, or the functional ones when fscrypt is to be > used with the filesystem. > > Using USE_FS_ENCRYPTION is a relatively clean approach, and requires > minimal changes to the filesystems supporting fscrypt. This RFC is > mostly for checking the acceptance of this solution, or if an other > direction is preferred. > > --- > > Niels de Vos (4): > fscrypt: introduce USE_FS_ENCRYPTION > fs: make fscrypt support an ext4 config option > fs: make fscrypt support a f2fs config option > fs: make fscrypt support a UBIFS config option So as others have pointed out, it doesn't seem worth the complexity to do this. For a bit of historical context, before Linux v5.1, we did have per-filesystem options for this: CONFIG_EXT4_ENCRYPTION, CONFIG_F2FS_FS_ENCRYPTION, and CONFIG_UBIFS_FS_ENCRYPTION. If you enabled one of these, it selected CONFIG_FS_ENCRYPTION to get the code in fs/crypto/. CONFIG_FS_ENCRYPTION was a tristate, so the code in fs/crypto/ could be built as a loadable module if it was only needed by filesystems that were loadable modules themselves. Having fs/crypto/ possibly be a loadable module was problematic, though, because it made it impossible to call into fs/crypto/ from built-in code such as fs/buffer.c, fs/ioctl.c, fs/libfs.c, fs/super.c, fs/iomap/direct-io.c, etc. So that's why we made CONFIG_FS_ENCRYPTION into a bool. At the same time, we decided to simplify the kconfig options by removing the per-filesystem options so that it worked like CONFIG_QUOTA, CONFIG_FS_DAX, CONFIG_FS_POSIX_ACL, etc. I suppose we *could* have *just* changed CONFIG_FS_ENCRYPTION to a bool to solve the first problem, and kept the per-filesystem options. I think that wouldn't have made a lot of sense, though, for the reasons that Ted has already covered. A further point, beyond what Ted has already covered, is that non-filesystem-specific code can't honor filesystem-specific options. So e.g. if you had a filesystem with encryption disabled by kconfig, that then called into fs/iomap/direct-io.c to process an I/O request, it could potentially still call into fs/crypto/ to enable encryption on that I/O request, since fs/iomap/direct-io.c would think that encryption support is enabled. Granted, that *should* never actually happen, because this would only make a difference on encrypted files, and the filesystem shouldn't have allowed an encrypted file to be opened if it doesn't have encryption support enabled. But it does seem a bit odd, given that it would go against the goal of compiling out all encryption code for a filesystem. - Eric
On Thu, Nov 10, 2022 at 01:15:38PM -0500, Theodore Ts'o wrote: > On Thu, Nov 10, 2022 at 05:47:10PM +0100, Niels de Vos wrote: > > And, there actually are options like CONFIG_EXT4_FS_POSIX_ACL and > > CONFIG_EXT4_FS_SECURITY. Because these exist already, I did not expect > > too much concerns with proposing a CONFIG_EXT4_FS_ENCRYPTION... > > Actually, I was thinking of getting rid of them, as we've already > gotten rid of EXT4_FS_POSIX_ACL.... > > > Thanks for adding some history about this. I understand that extra > > options are needed while creating/tuning the filesystems. Preventing > > users from setting the right options in a filesystem is not easy, even > > if tools from a distribution do not offer setting the options. Disks can > > be portable, or network-attached, and have options enabled that an other > > distributions kernel does not (want to) support. > > Sure, but as I said, there are **tons** of file system features that > have not and/or still are not supported for distros, but for which we > don't have kernel config knobs. This includes ext4's bigalloc and > inline data, btrfs's dedup and reflink support, xfs online fsck, etc., > etc., etc. Heck, ext4 is only supported up to a certain size by Red > Hat, and we don't have a Kernel config so that the kernel will > absolutely refuse to mount an ext4 file system larger than The > Officially Supported RHEL Capacity Limit for Ext4. So what makes > fscrypt different from all of these other unsupported file system > features? > > There are plenty of times when I've had to explain to customers why, > sure they could build their own kernels for RHEL 4 (back in the day > when I worked for Big Blue and had to talk to lots of enterprise > customers), but if they did, Red Hat support would refuse to give them > the time of day if they called asking for help. We didn't set up use > digitally signed kernels with trusted boot so that a IBM server would > refuse to boot anything other than An Officially Signed RHEL > Kernel... > > What makes fscrypt different that we think we need to enforce this > using technical means, other than a simple, "this feature is not > supported"? Thanks again for the added details. What you are explaining makes sense, and I am not sure if there is an other good reason why splitting out fscrypt support per filesystem would be required. I'm checking with the folks that suggested doing this, and see where we go from there. Niels
On Sun, Nov 13, 2022 at 10:02:37PM -0800, Christoph Hellwig wrote: > On Thu, Nov 10, 2022 at 05:47:10PM +0100, Niels de Vos wrote: > > And, there actually are options like CONFIG_EXT4_FS_POSIX_ACL and > > CONFIG_EXT4_FS_SECURITY. Because these exist already, I did not expect > > too much concerns with proposing a CONFIG_EXT4_FS_ENCRYPTION... > > ext4 is a little weird there as most file systems don't do that. > So I think these should go away for ext4 as well. Yeah, I understand that there is a preference for reducing the number of Kconfig options for filesystems. That indeed would make it a little easier for users, so I am supportive of that as well. > > Note that even with the additional options, enabling only > > CONFIG_FS_ENCRYPTION causes all the filesystems that support fscrypt to > > have it enabled. For users there is no change, except that they now have > > an option to disable fscrypt support per filesystem. > > But why would you do that anyay? An other mail in this thread contains a description about that. It is more about being able to provide a kernel build that is fully tested, and enabling more options (or being unable to disable features) increases the testing efforts that are needed. However, as Ted pointed out, there are other features that can not be disabled or limited per filesystem, so there will always be a gap in what can practically be tested. Thanks, Niels
On Tue, Nov 15, 2022 at 06:10:59PM -0800, Eric Biggers wrote: > On Thu, Nov 10, 2022 at 03:12:21PM +0100, Niels de Vos wrote: > > While more filesystems are getting support for fscrypt, it is useful to > > be able to disable fscrypt for a selection of filesystems, while > > enabling it for others. > > > > The new USE_FS_ENCRYPTION define gets picked up in > > include/linux/fscrypt.h. This allows filesystems to choose to use the > > empty function definitions, or the functional ones when fscrypt is to be > > used with the filesystem. > > > > Using USE_FS_ENCRYPTION is a relatively clean approach, and requires > > minimal changes to the filesystems supporting fscrypt. This RFC is > > mostly for checking the acceptance of this solution, or if an other > > direction is preferred. > > > > --- > > > > Niels de Vos (4): > > fscrypt: introduce USE_FS_ENCRYPTION > > fs: make fscrypt support an ext4 config option > > fs: make fscrypt support a f2fs config option > > fs: make fscrypt support a UBIFS config option > > So as others have pointed out, it doesn't seem worth the complexity to do this. > > For a bit of historical context, before Linux v5.1, we did have per-filesystem > options for this: CONFIG_EXT4_ENCRYPTION, CONFIG_F2FS_FS_ENCRYPTION, and > CONFIG_UBIFS_FS_ENCRYPTION. If you enabled one of these, it selected > CONFIG_FS_ENCRYPTION to get the code in fs/crypto/. CONFIG_FS_ENCRYPTION was a > tristate, so the code in fs/crypto/ could be built as a loadable module if it > was only needed by filesystems that were loadable modules themselves. > > Having fs/crypto/ possibly be a loadable module was problematic, though, because > it made it impossible to call into fs/crypto/ from built-in code such as > fs/buffer.c, fs/ioctl.c, fs/libfs.c, fs/super.c, fs/iomap/direct-io.c, etc. So > that's why we made CONFIG_FS_ENCRYPTION into a bool. At the same time, we > decided to simplify the kconfig options by removing the per-filesystem options > so that it worked like CONFIG_QUOTA, CONFIG_FS_DAX, CONFIG_FS_POSIX_ACL, etc. > > I suppose we *could* have *just* changed CONFIG_FS_ENCRYPTION to a bool to solve > the first problem, and kept the per-filesystem options. I think that wouldn't > have made a lot of sense, though, for the reasons that Ted has already covered. Yes, it seems that there is a move to reduce the Kconfig options and (re)adding per-filesystem encryption support would be counterproductive. > A further point, beyond what Ted has already covered, is that > non-filesystem-specific code can't honor filesystem-specific options. So e.g. > if you had a filesystem with encryption disabled by kconfig, that then called > into fs/iomap/direct-io.c to process an I/O request, it could potentially still > call into fs/crypto/ to enable encryption on that I/O request, since > fs/iomap/direct-io.c would think that encryption support is enabled. > > Granted, that *should* never actually happen, because this would only make a > difference on encrypted files, and the filesystem shouldn't have allowed an > encrypted file to be opened if it doesn't have encryption support enabled. But > it does seem a bit odd, given that it would go against the goal of compiling out > all encryption code for a filesystem. Ah, yes, indeed! The boundaries between the options would be less clear, and potential changes to shared functions under fs/ could have incorrect assumptions about CONFIG_FS_ENCRYPTION. Even if this is not the case now, optimizations/enhancements in the future might be more complicated because of this. Thanks for the additional details! Have a good weekend, Niels