Re: [RFC PATCH v10 10/48] ceph: implement -o test_dummy_encryption mount option

["Luís Henriques" <lhenriques@suse.de>]

Jeff Layton <jlayton@kernel.org> writes:

> On Fri, 2022-02-11 at 13:50 +0000, Luís Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> writes:
>> 
>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> > ---
>> >  fs/ceph/crypto.c | 53 ++++++++++++++++++++++++++++++++
>> >  fs/ceph/crypto.h | 26 ++++++++++++++++
>> >  fs/ceph/inode.c  | 10 ++++--
>> >  fs/ceph/super.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
>> >  fs/ceph/super.h  | 12 +++++++-
>> >  fs/ceph/xattr.c  |  3 ++
>> >  6 files changed, 177 insertions(+), 6 deletions(-)
>> > 
>> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> > index a513ff373b13..017f31eacb74 100644
>> > --- a/fs/ceph/crypto.c
>> > +++ b/fs/ceph/crypto.c
>> > @@ -4,6 +4,7 @@
>> >  #include <linux/fscrypt.h>
>> >  
>> >  #include "super.h"
>> > +#include "mds_client.h"
>> >  #include "crypto.h"
>> >  
>> >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>> > @@ -64,9 +65,20 @@ static bool ceph_crypt_empty_dir(struct inode *inode)
>> >  	return ci->i_rsubdirs + ci->i_rfiles == 1;
>> >  }
>> >  
>> > +void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc)
>> > +{
>> > +	fscrypt_free_dummy_policy(&fsc->dummy_enc_policy);
>> > +}
>> > +
>> > +static const union fscrypt_policy *ceph_get_dummy_policy(struct super_block *sb)
>> > +{
>> > +	return ceph_sb_to_client(sb)->dummy_enc_policy.policy;
>> > +}
>> > +
>> >  static struct fscrypt_operations ceph_fscrypt_ops = {
>> >  	.get_context		= ceph_crypt_get_context,
>> >  	.set_context		= ceph_crypt_set_context,
>> > +	.get_dummy_policy	= ceph_get_dummy_policy,
>> >  	.empty_dir		= ceph_crypt_empty_dir,
>> >  };
>> >  
>> > @@ -74,3 +86,44 @@ void ceph_fscrypt_set_ops(struct super_block *sb)
>> >  {
>> >  	fscrypt_set_ops(sb, &ceph_fscrypt_ops);
>> >  }
>> > +
>> > +int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>> > +				 struct ceph_acl_sec_ctx *as)
>> > +{
>> > +	int ret, ctxsize;
>> > +	bool encrypted = false;
>> > +	struct ceph_inode_info *ci = ceph_inode(inode);
>> > +
>> > +	ret = fscrypt_prepare_new_inode(dir, inode, &encrypted);
>> > +	if (ret)
>> > +		return ret;
>> > +	if (!encrypted)
>> > +		return 0;
>> > +
>> > +	as->fscrypt_auth = kzalloc(sizeof(*as->fscrypt_auth), GFP_KERNEL);
>> > +	if (!as->fscrypt_auth)
>> > +		return -ENOMEM;
>> > +
>> 
>> Isn't this memory allocation leaking bellow in the error paths?
>> 
>> (Yeah, I'm finally (but slowly) catching up with this series... my memory
>> is blurry and there are a lot of things I forgot...)
>> 
>> Cheers,
>
> No. If an error bubbles back up here, we'll eventually call
> ceph_release_acl_sec_ctx on the thing, and it'll be kfreed then.

Right, the callers are expected to ensure that ceph_release_acl_sec_ctx()
is invoked, of course.  Sorry for the noise :-/

Cheers,
-- 
Luís