| Message ID | 20211209153647.58953-6-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ceph+fscrypt: context, filename, symlink and size handling support | expand |
On Thu, Dec 09, 2021 at 10:36:16AM -0500, Jeff Layton wrote: > ceph_atomic_open needs to be able to call this. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/crypto/fscrypt_private.h | 26 -------------------------- > fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ > include/linux/fscrypt.h | 5 +++++ > 3 files changed, 32 insertions(+), 26 deletions(-) What is the use case for this, more precisely? I've been trying to keep filesystems using helper functions like fscrypt_prepare_*() and fscrypt_file_open() rather than setting up encryption keys directly, which is a bit too low-level to be doing outside of fs/crypto/. Perhaps fscrypt_file_open() is what you're looking for here? - Eric
On Fri, 2021-12-10 at 11:46 -0800, Eric Biggers wrote: > On Thu, Dec 09, 2021 at 10:36:16AM -0500, Jeff Layton wrote: > > ceph_atomic_open needs to be able to call this. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/crypto/fscrypt_private.h | 26 -------------------------- > > fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ > > include/linux/fscrypt.h | 5 +++++ > > 3 files changed, 32 insertions(+), 26 deletions(-) > > What is the use case for this, more precisely? I've been trying to keep > filesystems using helper functions like fscrypt_prepare_*() and > fscrypt_file_open() rather than setting up encryption keys directly, which is a > bit too low-level to be doing outside of fs/crypto/. > > Perhaps fscrypt_file_open() is what you're looking for here? That doesn't really help because we don't have the inode for the file yet at the point where we need the key. atomic_open basically does a lookup+open. You give it a directory inode and a dentry, and it issues an open request by filename. If it gets back ENOENT then we know that the thing is a negative dentry. In the lookup path, I used __fscrypt_prepare_readdir. This situation is a bit similar so I might be able to use that instead. OTOH, that doesn't fail when you don't have the key, and if you don't, there's not a lot of point in going any further here.
On Fri, Dec 10, 2021 at 03:40:20PM -0500, Jeff Layton wrote: > On Fri, 2021-12-10 at 11:46 -0800, Eric Biggers wrote: > > On Thu, Dec 09, 2021 at 10:36:16AM -0500, Jeff Layton wrote: > > > ceph_atomic_open needs to be able to call this. > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/crypto/fscrypt_private.h | 26 -------------------------- > > > fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ > > > include/linux/fscrypt.h | 5 +++++ > > > 3 files changed, 32 insertions(+), 26 deletions(-) > > > > What is the use case for this, more precisely? I've been trying to keep > > filesystems using helper functions like fscrypt_prepare_*() and > > fscrypt_file_open() rather than setting up encryption keys directly, which is a > > bit too low-level to be doing outside of fs/crypto/. > > > > Perhaps fscrypt_file_open() is what you're looking for here? > > That doesn't really help because we don't have the inode for the file > yet at the point where we need the key. > > atomic_open basically does a lookup+open. You give it a directory inode > and a dentry, and it issues an open request by filename. If it gets back > ENOENT then we know that the thing is a negative dentry. > > In the lookup path, I used __fscrypt_prepare_readdir. This situation is > a bit similar so I might be able to use that instead. OTOH, that doesn't > fail when you don't have the key, and if you don't, there's not a lot of > point in going any further here. So you're requiring the key on a directory to do a lookup in that directory? Normally that's not required, as files can be looked up by no-key name. Why is the atomic_open case different? The file inode's key is needed to open it, of course, but the directory inode's key shouldn't be needed. In practice you'll tend to have the key for both or neither inode, but that's not guaranteed. - Eric
On Sun, 2021-12-12 at 11:56 -0800, Eric Biggers wrote: > On Fri, Dec 10, 2021 at 03:40:20PM -0500, Jeff Layton wrote: > > On Fri, 2021-12-10 at 11:46 -0800, Eric Biggers wrote: > > > On Thu, Dec 09, 2021 at 10:36:16AM -0500, Jeff Layton wrote: > > > > ceph_atomic_open needs to be able to call this. > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > fs/crypto/fscrypt_private.h | 26 -------------------------- > > > > fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ > > > > include/linux/fscrypt.h | 5 +++++ > > > > 3 files changed, 32 insertions(+), 26 deletions(-) > > > > > > What is the use case for this, more precisely? I've been trying to keep > > > filesystems using helper functions like fscrypt_prepare_*() and > > > fscrypt_file_open() rather than setting up encryption keys directly, which is a > > > bit too low-level to be doing outside of fs/crypto/. > > > > > > Perhaps fscrypt_file_open() is what you're looking for here? > > > > That doesn't really help because we don't have the inode for the file > > yet at the point where we need the key. > > > > atomic_open basically does a lookup+open. You give it a directory inode > > and a dentry, and it issues an open request by filename. If it gets back > > ENOENT then we know that the thing is a negative dentry. > > > > In the lookup path, I used __fscrypt_prepare_readdir. This situation is > > a bit similar so I might be able to use that instead. OTOH, that doesn't > > fail when you don't have the key, and if you don't, there's not a lot of > > point in going any further here. > > So you're requiring the key on a directory to do a lookup in that directory? > Normally that's not required, as files can be looked up by no-key name. Why is > the atomic_open case different? > > The file inode's key is needed to open it, of > course, but the directory inode's key shouldn't be needed. In practice you'll > tend to have the key for both or neither inode, but that's not guaranteed. > We're issuing an open request to the server without looking up the inode first. In order to do that open request, we need to encode a filename into the request, and to do that we need the encryption key.
On Sun, Dec 12, 2021 at 03:38:29PM -0500, Jeff Layton wrote: > On Sun, 2021-12-12 at 11:56 -0800, Eric Biggers wrote: > > On Fri, Dec 10, 2021 at 03:40:20PM -0500, Jeff Layton wrote: > > > On Fri, 2021-12-10 at 11:46 -0800, Eric Biggers wrote: > > > > On Thu, Dec 09, 2021 at 10:36:16AM -0500, Jeff Layton wrote: > > > > > ceph_atomic_open needs to be able to call this. > > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > --- > > > > > fs/crypto/fscrypt_private.h | 26 -------------------------- > > > > > fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ > > > > > include/linux/fscrypt.h | 5 +++++ > > > > > 3 files changed, 32 insertions(+), 26 deletions(-) > > > > > > > > What is the use case for this, more precisely? I've been trying to keep > > > > filesystems using helper functions like fscrypt_prepare_*() and > > > > fscrypt_file_open() rather than setting up encryption keys directly, which is a > > > > bit too low-level to be doing outside of fs/crypto/. > > > > > > > > Perhaps fscrypt_file_open() is what you're looking for here? > > > > > > That doesn't really help because we don't have the inode for the file > > > yet at the point where we need the key. > > > > > > atomic_open basically does a lookup+open. You give it a directory inode > > > and a dentry, and it issues an open request by filename. If it gets back > > > ENOENT then we know that the thing is a negative dentry. > > > > > > In the lookup path, I used __fscrypt_prepare_readdir. This situation is > > > a bit similar so I might be able to use that instead. OTOH, that doesn't > > > fail when you don't have the key, and if you don't, there's not a lot of > > > point in going any further here. > > > > So you're requiring the key on a directory to do a lookup in that directory? > > Normally that's not required, as files can be looked up by no-key name. Why is > > the atomic_open case different? > > > > The file inode's key is needed to open it, of > > course, but the directory inode's key shouldn't be needed. In practice you'll > > tend to have the key for both or neither inode, but that's not guaranteed. > > > > We're issuing an open request to the server without looking up the inode > first. In order to do that open request, we need to encode a filename > into the request, and to do that we need the encryption key. But how is it different from a regular lookup? Those try to set up the directory's key, but if it's unavailable, the name being looked up is treated as a no-key name. Take a look at fscrypt_prepare_lookup(). - Eric
On Sun, 2021-12-12 at 13:03 -0800, Eric Biggers wrote: > On Sun, Dec 12, 2021 at 03:38:29PM -0500, Jeff Layton wrote: > > On Sun, 2021-12-12 at 11:56 -0800, Eric Biggers wrote: > > > On Fri, Dec 10, 2021 at 03:40:20PM -0500, Jeff Layton wrote: > > > > On Fri, 2021-12-10 at 11:46 -0800, Eric Biggers wrote: > > > > > On Thu, Dec 09, 2021 at 10:36:16AM -0500, Jeff Layton wrote: > > > > > > ceph_atomic_open needs to be able to call this. > > > > > > > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > --- > > > > > > fs/crypto/fscrypt_private.h | 26 -------------------------- > > > > > > fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ > > > > > > include/linux/fscrypt.h | 5 +++++ > > > > > > 3 files changed, 32 insertions(+), 26 deletions(-) > > > > > > > > > > What is the use case for this, more precisely? I've been trying to keep > > > > > filesystems using helper functions like fscrypt_prepare_*() and > > > > > fscrypt_file_open() rather than setting up encryption keys directly, which is a > > > > > bit too low-level to be doing outside of fs/crypto/. > > > > > > > > > > Perhaps fscrypt_file_open() is what you're looking for here? > > > > > > > > That doesn't really help because we don't have the inode for the file > > > > yet at the point where we need the key. > > > > > > > > atomic_open basically does a lookup+open. You give it a directory inode > > > > and a dentry, and it issues an open request by filename. If it gets back > > > > ENOENT then we know that the thing is a negative dentry. > > > > > > > > In the lookup path, I used __fscrypt_prepare_readdir. This situation is > > > > a bit similar so I might be able to use that instead. OTOH, that doesn't > > > > fail when you don't have the key, and if you don't, there's not a lot of > > > > point in going any further here. > > > > > > So you're requiring the key on a directory to do a lookup in that directory? > > > Normally that's not required, as files can be looked up by no-key name. Why is > > > the atomic_open case different? > > > > > > The file inode's key is needed to open it, of > > > course, but the directory inode's key shouldn't be needed. In practice you'll > > > tend to have the key for both or neither inode, but that's not guaranteed. > > > > > > > We're issuing an open request to the server without looking up the inode > > first. In order to do that open request, we need to encode a filename > > into the request, and to do that we need the encryption key. > > But how is it different from a regular lookup? Those try to set up the > directory's key, but if it's unavailable, the name being looked up is treated as > a no-key name. Take a look at fscrypt_prepare_lookup(). > Ok. After looking at this some more, I think you're right. I don't really need this call in atomic_open at all. I'll plan to just drop this patch from the series. Thanks!
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index 51e42767dbd6..89d5d85afdd5 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -579,32 +579,6 @@ void fscrypt_hash_inode_number(struct fscrypt_info *ci, int fscrypt_get_encryption_info(struct inode *inode, bool allow_unsupported); -/** - * fscrypt_require_key() - require an inode's encryption key - * @inode: the inode we need the key for - * - * If the inode is encrypted, set up its encryption key if not already done. - * Then require that the key be present and return -ENOKEY otherwise. - * - * No locks are needed, and the key will live as long as the struct inode --- so - * it won't go away from under you. - * - * Return: 0 on success, -ENOKEY if the key is missing, or another -errno code - * if a problem occurred while setting up the encryption key. - */ -static inline int fscrypt_require_key(struct inode *inode) -{ - if (IS_ENCRYPTED(inode)) { - int err = fscrypt_get_encryption_info(inode, false); - - if (err) - return err; - if (!fscrypt_has_encryption_key(inode)) - return -ENOKEY; - } - return 0; -} - /* keysetup_v1.c */ void fscrypt_put_direct_key(struct fscrypt_direct_key *dk); diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index eede186b04ce..7aeb0047d03d 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -792,3 +792,30 @@ int fscrypt_drop_inode(struct inode *inode) return !is_master_key_secret_present(&mk->mk_secret); } EXPORT_SYMBOL_GPL(fscrypt_drop_inode); + +/** + * fscrypt_require_key() - require an inode's encryption key + * @inode: the inode we need the key for + * + * If the inode is encrypted, set up its encryption key if not already done. + * Then require that the key be present and return -ENOKEY otherwise. + * + * No locks are needed, and the key will live as long as the struct inode --- so + * it won't go away from under you. + * + * Return: 0 on success, -ENOKEY if the key is missing, or another -errno code + * if a problem occurred while setting up the encryption key. + */ +int fscrypt_require_key(struct inode *inode) +{ + if (IS_ENCRYPTED(inode)) { + int err = fscrypt_get_encryption_info(inode, false); + + if (err) + return err; + if (!fscrypt_has_encryption_key(inode)) + return -ENOKEY; + } + return 0; +} +EXPORT_SYMBOL_GPL(fscrypt_require_key); diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 530433098f82..7d3c670d0c6d 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -334,6 +334,7 @@ bool fscrypt_match_name(const struct fscrypt_name *fname, const u8 *de_name, u32 de_name_len); u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name); int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags); +int fscrypt_require_key(struct inode *inode); /* bio.c */ void fscrypt_decrypt_bio(struct bio *bio); @@ -601,6 +602,10 @@ static inline int fscrypt_d_revalidate(struct dentry *dentry, return 1; } +static inline int fscrypt_require_key(struct inode *inode) +{ + return 0; +} /* bio.c */ static inline void fscrypt_decrypt_bio(struct bio *bio) {
ceph_atomic_open needs to be able to call this. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/crypto/fscrypt_private.h | 26 -------------------------- fs/crypto/keysetup.c | 27 +++++++++++++++++++++++++++ include/linux/fscrypt.h | 5 +++++ 3 files changed, 32 insertions(+), 26 deletions(-)