| Message ID | 20200904160537.76663-5-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph+fscrypt: context, filename and symlink support | expand |
On Fri, Sep 04, 2020 at 12:05:23PM -0400, Jeff Layton wrote: > CephFS will need to be able to generate a context for a new "prepared" > inode. Add a new routine for getting the context out of an in-core > inode. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/crypto/policy.c | 20 ++++++++++++++++++++ > include/linux/fscrypt.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > index c56ad886f7d7..10eddd113a21 100644 > --- a/fs/crypto/policy.c > +++ b/fs/crypto/policy.c > @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data) > } > EXPORT_SYMBOL_GPL(fscrypt_set_context); > > +/** > + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode Comment doesn't match the function name. Also, the name isn't very clear. How about calling this fscrypt_context_for_new_inode()? BTW, I might rename fscrypt_new_context_from_policy() to fscrypt_context_from_policy() in my patchset. Since it now makes the caller provide the nonce, technically it's no longer limited to "new" contexts. > + * @ctx: where context should be written > + * @inode: inode from which to fetch context > + * > + * Given an in-core prepared, but not-necessarily fully-instantiated inode, > + * generate an encryption context from its policy and write it to ctx. Clarify what is meant by "prepared" (fscrypt_prepare_new_inode() was called) vs. "instantiated". > + * > + * Returns size of the context. > + */ > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode) > +{ > + struct fscrypt_info *ci = inode->i_crypt_info; > + > + BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE); > + > + return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce); > +} > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode); fscrypt_set_context() should be changed to call this, instead of duplicating the same logic. As part of that, the WARN_ON_ONCE(!ci) that's currently in fscrypt_set_context() should go in here instead. - Eric
On Mon, 2020-09-07 at 20:48 -0700, Eric Biggers wrote: > On Fri, Sep 04, 2020 at 12:05:23PM -0400, Jeff Layton wrote: > > CephFS will need to be able to generate a context for a new "prepared" > > inode. Add a new routine for getting the context out of an in-core > > inode. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/crypto/policy.c | 20 ++++++++++++++++++++ > > include/linux/fscrypt.h | 1 + > > 2 files changed, 21 insertions(+) > > > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > > index c56ad886f7d7..10eddd113a21 100644 > > --- a/fs/crypto/policy.c > > +++ b/fs/crypto/policy.c > > @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data) > > } > > EXPORT_SYMBOL_GPL(fscrypt_set_context); > > > > +/** > > + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode > > Comment doesn't match the function name. > > Also, the name isn't very clear. How about calling this > fscrypt_context_for_new_inode()? > > BTW, I might rename fscrypt_new_context_from_policy() to > fscrypt_context_from_policy() in my patchset. Since it now makes the caller > provide the nonce, technically it's no longer limited to "new" contexts. > Ahh yes. I didn't properly update the commit message here. Your suggested names sound fine. I'll plan to fix that up. > > + * @ctx: where context should be written > > + * @inode: inode from which to fetch context > > + * > > + * Given an in-core prepared, but not-necessarily fully-instantiated inode, > > + * generate an encryption context from its policy and write it to ctx. > > Clarify what is meant by "prepared" (fscrypt_prepare_new_inode() was called) > vs. "instantiated". > Ack. > > + * > > + * Returns size of the context. > > + */ > > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode) > > +{ > > + struct fscrypt_info *ci = inode->i_crypt_info; > > + > > + BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE); > > + > > + return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce); > > +} > > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode); > > fscrypt_set_context() should be changed to call this, instead of duplicating the > same logic. As part of that, the WARN_ON_ONCE(!ci) that's currently in > fscrypt_set_context() should go in here instead. Ok.
On Mon, 2020-09-07 at 20:48 -0700, Eric Biggers wrote: > On Fri, Sep 04, 2020 at 12:05:23PM -0400, Jeff Layton wrote: > > CephFS will need to be able to generate a context for a new "prepared" > > inode. Add a new routine for getting the context out of an in-core > > inode. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/crypto/policy.c | 20 ++++++++++++++++++++ > > include/linux/fscrypt.h | 1 + > > 2 files changed, 21 insertions(+) > > > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > > index c56ad886f7d7..10eddd113a21 100644 > > --- a/fs/crypto/policy.c > > +++ b/fs/crypto/policy.c > > @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data) > > } > > EXPORT_SYMBOL_GPL(fscrypt_set_context); > > > > +/** > > + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode > > Comment doesn't match the function name. > > Also, the name isn't very clear. How about calling this > fscrypt_context_for_new_inode()? > > BTW, I might rename fscrypt_new_context_from_policy() to > fscrypt_context_from_policy() in my patchset. Since it now makes the caller > provide the nonce, technically it's no longer limited to "new" contexts. > > > + * @ctx: where context should be written > > + * @inode: inode from which to fetch context > > + * > > + * Given an in-core prepared, but not-necessarily fully-instantiated inode, > > + * generate an encryption context from its policy and write it to ctx. > > Clarify what is meant by "prepared" (fscrypt_prepare_new_inode() was called) > vs. "instantiated". > > > + * > > + * Returns size of the context. > > + */ > > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode) > > +{ > > + struct fscrypt_info *ci = inode->i_crypt_info; > > + > > + BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE); > > + > > + return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce); > > +} > > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode); > > fscrypt_set_context() should be changed to call this, instead of duplicating the > same logic. As part of that, the WARN_ON_ONCE(!ci) that's currently in > fscrypt_set_context() should go in here instead. > Note that we can't just move that WARN_ON_ONCE. If we do that, then fscrypt_set_context will dereference ci before that check can occur, so we'd be trading a warning and -ENOKEY for a NULL pointer dereference. I think we'll have to duplicate that in both functions.
On Tue, Sep 08, 2020 at 08:29:52AM -0400, Jeff Layton wrote: > > > + * > > > + * Returns size of the context. > > > + */ > > > +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode) > > > +{ > > > + struct fscrypt_info *ci = inode->i_crypt_info; > > > + > > > + BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE); > > > + > > > + return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce); > > > +} > > > +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode); > > > > fscrypt_set_context() should be changed to call this, instead of duplicating the > > same logic. As part of that, the WARN_ON_ONCE(!ci) that's currently in > > fscrypt_set_context() should go in here instead. > > > > Note that we can't just move that WARN_ON_ONCE. If we do that, then > fscrypt_set_context will dereference ci before that check can occur, so > we'd be trading a warning and -ENOKEY for a NULL pointer dereference. I > think we'll have to duplicate that in both functions. You could just make fscrypt_set_context() call fscrypt_new_context_from_inode() first, before the fscrypt_hash_inode_number() stuff. - Eric
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c index c56ad886f7d7..10eddd113a21 100644 --- a/fs/crypto/policy.c +++ b/fs/crypto/policy.c @@ -670,6 +670,26 @@ int fscrypt_set_context(struct inode *inode, void *fs_data) } EXPORT_SYMBOL_GPL(fscrypt_set_context); +/** + * fscrypt_context_from_inode() - fetch the encryption context out of in-core inode + * @ctx: where context should be written + * @inode: inode from which to fetch context + * + * Given an in-core prepared, but not-necessarily fully-instantiated inode, + * generate an encryption context from its policy and write it to ctx. + * + * Returns size of the context. + */ +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode) +{ + struct fscrypt_info *ci = inode->i_crypt_info; + + BUILD_BUG_ON(sizeof(*ctx) != FSCRYPT_SET_CONTEXT_MAX_SIZE); + + return fscrypt_new_context_from_policy(ctx, &ci->ci_policy, ci->ci_nonce); +} +EXPORT_SYMBOL_GPL(fscrypt_new_context_from_inode); + /** * fscrypt_set_test_dummy_encryption() - handle '-o test_dummy_encryption' * @sb: the filesystem on which test_dummy_encryption is being specified diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 16d673c50448..0ddbd27a2e58 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -157,6 +157,7 @@ int fscrypt_ioctl_get_policy_ex(struct file *filp, void __user *arg); int fscrypt_ioctl_get_nonce(struct file *filp, void __user *arg); int fscrypt_has_permitted_context(struct inode *parent, struct inode *child); int fscrypt_set_context(struct inode *inode, void *fs_data); +int fscrypt_new_context_from_inode(union fscrypt_context *ctx, struct inode *inode); struct fscrypt_dummy_context { const union fscrypt_context *ctx;
CephFS will need to be able to generate a context for a new "prepared" inode. Add a new routine for getting the context out of an in-core inode. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/crypto/policy.c | 20 ++++++++++++++++++++ include/linux/fscrypt.h | 1 + 2 files changed, 21 insertions(+)