| Message ID | 20170920224605.22030-1-ebiggers3@gmail.com |
|---|---|
| Headers | show |
| Series | fscrypt: add some higher-level helper functions | expand |
On Wed, Sep 20, 2017 at 03:45:40PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > This series reduces code duplication among ext4, f2fs, and ubifs by > introducing a S_ENCRYPTED inode flag (so we don't have to call back into > the filesystem to test the filesystem-specific inode flag), then > introducing new helper functions that are called at the beginning of the > open, link, rename, lookup, and setattr operations. > > In the future we maybe should even call these new helpers from the VFS > so that each individual filesystem doesn't have to do it. But that's > not possible currently because fs/crypto/ can be built as a module. > > Making changes like this is a bit challenging due to interdependencies > between fscrypt and the individual filesystems, all of which have > different maintainers. For now my intent is that patches 1-10 be taken > through the fscrypt tree --- though it's not perfect since patches 1-4 > do make some changes to each filesystem, as everyone must set > S_ENCRYPTED before we can use it everywhere in the shared code. But > afterwards, patches 11-25 can be picked up by the individual filesystems > to switch to the new helpers. This all looks much nicer. Having just been looking at this stuff, it makes the code much simpler to understand. So: Acked-by: Dave Chinner <dchinner@redhat.com> While I'm here, the fscrypt header file includes are clunky and nasty. I worte a quick patch a couple of days ago to clean it up. See below.... Cheers, Dave.
On 2017/9/21 6:45, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > This series reduces code duplication among ext4, f2fs, and ubifs by > introducing a S_ENCRYPTED inode flag (so we don't have to call back into > the filesystem to test the filesystem-specific inode flag), then > introducing new helper functions that are called at the beginning of the > open, link, rename, lookup, and setattr operations. > > In the future we maybe should even call these new helpers from the VFS > so that each individual filesystem doesn't have to do it. But that's > not possible currently because fs/crypto/ can be built as a module. > > Making changes like this is a bit challenging due to interdependencies > between fscrypt and the individual filesystems, all of which have > different maintainers. For now my intent is that patches 1-10 be taken > through the fscrypt tree --- though it's not perfect since patches 1-4 > do make some changes to each filesystem, as everyone must set > S_ENCRYPTED before we can use it everywhere in the shared code. But > afterwards, patches 11-25 can be picked up by the individual filesystems > to switch to the new helpers. For all patches touching f2fs, looks good to me, feel free to add: Reviewed-by: Chao Yu <yuchao0@huawei.com> > > Eric Biggers (25): > fs, fscrypt: add an S_ENCRYPTED inode flag > fscrypt: switch from ->is_encrypted() to IS_ENCRYPTED() > fscrypt: remove ->is_encrypted() > fscrypt: remove unneeded empty fscrypt_operations structs > fscrypt: new helper function - fscrypt_require_key() > fscrypt: new helper function - fscrypt_file_open() > fscrypt: new helper function - fscrypt_prepare_link() > fscrypt: new helper function - fscrypt_prepare_rename() > fscrypt: new helper function - fscrypt_prepare_lookup() > fscrypt: new helper function - fscrypt_prepare_setattr() > ext4: switch to fscrypt_file_open() > ext4: switch to fscrypt_prepare_link() > ext4: switch to fscrypt_prepare_rename() > ext4: switch to fscrypt_prepare_lookup() > ext4: switch to fscrypt_prepare_setattr() > f2fs: switch to fscrypt_file_open() > f2fs: switch to fscrypt_prepare_link() > f2fs: switch to fscrypt_prepare_rename() > f2fs: switch to fscrypt_prepare_lookup() > f2fs: switch to fscrypt_prepare_setattr() > ubifs: switch to fscrypt_file_open() > ubifs: switch to fscrypt_prepare_link() > ubifs: switch to fscrypt_prepare_rename() > ubifs: switch to fscrypt_prepare_lookup() > ubifs: switch to fscrypt_prepare_setattr() > > fs/crypto/Makefile | 2 +- > fs/crypto/crypto.c | 2 +- > fs/crypto/fname.c | 3 +- > fs/crypto/hooks.c | 112 +++++++++++++++++++++++++++++ > fs/crypto/keyinfo.c | 2 +- > fs/crypto/policy.c | 6 +- > fs/ext4/file.c | 23 ++---- > fs/ext4/inode.c | 19 +++-- > fs/ext4/namei.c | 62 +++++----------- > fs/ext4/super.c | 15 ++-- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/file.c | 30 ++------ > fs/f2fs/inode.c | 5 +- > fs/f2fs/namei.c | 54 ++++---------- > fs/f2fs/super.c | 7 +- > fs/ubifs/crypto.c | 1 - > fs/ubifs/dir.c | 43 ++++------- > fs/ubifs/file.c | 41 ++--------- > fs/ubifs/ioctl.c | 5 +- > fs/ubifs/super.c | 8 +-- > fs/ubifs/ubifs.h | 9 +-- > fs/ubifs/xattr.c | 1 + > include/linux/fs.h | 2 + > include/linux/fscrypt_common.h | 1 - > include/linux/fscrypt_notsupp.h | 54 +++++++++++++- > include/linux/fscrypt_supp.h | 153 ++++++++++++++++++++++++++++++++++++++++ > 26 files changed, 418 insertions(+), 243 deletions(-) > create mode 100644 fs/crypto/hooks.c >
Hi Dave, On Thu, Sep 21, 2017 at 04:45:02PM +1000, Dave Chinner wrote: > fscrypto: clean up include file mess > > From: Dave Chinner <dchinner@redhat.com> > > Filesystems have to include different header files based on whether > they are compiled with encryption support or not. That's nasty and > messy. > > Instead, rationalise the headers so we have a single include > fscrypt.h and let it decide what internal implementation to include > based on the __FS_HAS_ENCRYPTION define. Filesystems set > __FS_HAS_ENCRYPTION before including linux/fscrypt.h if they are > built with encryption support. > > Add guards to prevent fscrypt_supp.h and fscrypt_notsupp.h from > being directly included by filesystems. This looks good; we probably should have done it that way originally. This will allow us to have the inline functions like fscrypt_prepare_rename() defined in fscrypt.h, and then have supp/notsupp versions of __fscrypt_prepare_rename() instead --- so common checks like for IS_ENCRYPTED() will be in one place only. One nit: > +#ifdef CONFIG_EXT4_FS_ENCRYPTION > +#define __FS_HAS_ENCRYPTION 1 > +#endif > +#include <linux/fscrypt.h> How about doing #define __FS_HAS_ENCRYPTION IS_ENABLED(CONFIG_EXT4_FS_ENCRYPTION) (and likewise for f2fs and ubifs), then checking '#if __FS_HAS_ENCRYPTION' rather than '#ifdef __FS_HAS_ENCRYPTION'? Eric
On Thu, Sep 21, 2017 at 10:47:05AM -0700, Eric Biggers wrote: > Hi Dave, > > On Thu, Sep 21, 2017 at 04:45:02PM +1000, Dave Chinner wrote: > > fscrypto: clean up include file mess > > > > From: Dave Chinner <dchinner@redhat.com> > > > > Filesystems have to include different header files based on whether > > they are compiled with encryption support or not. That's nasty and > > messy. > > > > Instead, rationalise the headers so we have a single include > > fscrypt.h and let it decide what internal implementation to include > > based on the __FS_HAS_ENCRYPTION define. Filesystems set > > __FS_HAS_ENCRYPTION before including linux/fscrypt.h if they are > > built with encryption support. > > > > Add guards to prevent fscrypt_supp.h and fscrypt_notsupp.h from > > being directly included by filesystems. > > This looks good; we probably should have done it that way originally. This will > allow us to have the inline functions like fscrypt_prepare_rename() defined in > fscrypt.h, and then have supp/notsupp versions of __fscrypt_prepare_rename() > instead --- so common checks like for IS_ENCRYPTED() will be in one place only. *nod* > One nit: > > > +#ifdef CONFIG_EXT4_FS_ENCRYPTION > > +#define __FS_HAS_ENCRYPTION 1 > > +#endif > > +#include <linux/fscrypt.h> > > How about doing > > #define __FS_HAS_ENCRYPTION IS_ENABLED(CONFIG_EXT4_FS_ENCRYPTION) > > (and likewise for f2fs and ubifs), then checking '#if __FS_HAS_ENCRYPTION' > rather than '#ifdef __FS_HAS_ENCRYPTION'? Yeah, that's cleaner. I'll modify it and resend as a standalone patch. Cheers, Dave.
From: Eric Biggers <ebiggers@google.com> This series reduces code duplication among ext4, f2fs, and ubifs by introducing a S_ENCRYPTED inode flag (so we don't have to call back into the filesystem to test the filesystem-specific inode flag), then introducing new helper functions that are called at the beginning of the open, link, rename, lookup, and setattr operations. In the future we maybe should even call these new helpers from the VFS so that each individual filesystem doesn't have to do it. But that's not possible currently because fs/crypto/ can be built as a module. Making changes like this is a bit challenging due to interdependencies between fscrypt and the individual filesystems, all of which have different maintainers. For now my intent is that patches 1-10 be taken through the fscrypt tree --- though it's not perfect since patches 1-4 do make some changes to each filesystem, as everyone must set S_ENCRYPTED before we can use it everywhere in the shared code. But afterwards, patches 11-25 can be picked up by the individual filesystems to switch to the new helpers. Eric Biggers (25): fs, fscrypt: add an S_ENCRYPTED inode flag fscrypt: switch from ->is_encrypted() to IS_ENCRYPTED() fscrypt: remove ->is_encrypted() fscrypt: remove unneeded empty fscrypt_operations structs fscrypt: new helper function - fscrypt_require_key() fscrypt: new helper function - fscrypt_file_open() fscrypt: new helper function - fscrypt_prepare_link() fscrypt: new helper function - fscrypt_prepare_rename() fscrypt: new helper function - fscrypt_prepare_lookup() fscrypt: new helper function - fscrypt_prepare_setattr() ext4: switch to fscrypt_file_open() ext4: switch to fscrypt_prepare_link() ext4: switch to fscrypt_prepare_rename() ext4: switch to fscrypt_prepare_lookup() ext4: switch to fscrypt_prepare_setattr() f2fs: switch to fscrypt_file_open() f2fs: switch to fscrypt_prepare_link() f2fs: switch to fscrypt_prepare_rename() f2fs: switch to fscrypt_prepare_lookup() f2fs: switch to fscrypt_prepare_setattr() ubifs: switch to fscrypt_file_open() ubifs: switch to fscrypt_prepare_link() ubifs: switch to fscrypt_prepare_rename() ubifs: switch to fscrypt_prepare_lookup() ubifs: switch to fscrypt_prepare_setattr() fs/crypto/Makefile | 2 +- fs/crypto/crypto.c | 2 +- fs/crypto/fname.c | 3 +- fs/crypto/hooks.c | 112 +++++++++++++++++++++++++++++ fs/crypto/keyinfo.c | 2 +- fs/crypto/policy.c | 6 +- fs/ext4/file.c | 23 ++---- fs/ext4/inode.c | 19 +++-- fs/ext4/namei.c | 62 +++++----------- fs/ext4/super.c | 15 ++-- fs/f2fs/f2fs.h | 1 + fs/f2fs/file.c | 30 ++------ fs/f2fs/inode.c | 5 +- fs/f2fs/namei.c | 54 ++++---------- fs/f2fs/super.c | 7 +- fs/ubifs/crypto.c | 1 - fs/ubifs/dir.c | 43 ++++------- fs/ubifs/file.c | 41 ++--------- fs/ubifs/ioctl.c | 5 +- fs/ubifs/super.c | 8 +-- fs/ubifs/ubifs.h | 9 +-- fs/ubifs/xattr.c | 1 + include/linux/fs.h | 2 + include/linux/fscrypt_common.h | 1 - include/linux/fscrypt_notsupp.h | 54 +++++++++++++- include/linux/fscrypt_supp.h | 153 ++++++++++++++++++++++++++++++++++++++++ 26 files changed, 418 insertions(+), 243 deletions(-) create mode 100644 fs/crypto/hooks.c