Re: [RFC PATCH v7 07/24] ceph: add fscrypt_* handling to caps.c

[Xiubo Li <xiubli@redhat.com>]

On 7/7/21 8:02 PM, Jeff Layton wrote:
> On Wed, 2021-07-07 at 15:20 +0800, Xiubo Li wrote:
>> Hi Jeff,
>>
>> There has some following patches in your "fscrypt" branch, which is not
>> posted yet, the commit is:
>>
>> "3161d2f549db ceph: size handling for encrypted inodes in cap updates"
>>
>> It seems buggy.
>>
> Yes. Those are still quite rough. If I haven't posted them, then YMMV. I
> often push them to -experimental branches just for backup purposes. You
> may want to wait on reviewing those until I've had a chance to clean
> them up and post them.

Yeah, sure.

I am reviewing the code from you experimental branch.


>> In the encode_cap_msg() you have removed the 'fscrypt_file_len' and and
>> added a new 8 bytes' data encoding:
>>
>>           ceph_encode_32(&p, arg->fscrypt_auth_len);
>>           ceph_encode_copy(&p, arg->fscrypt_auth, arg->fscrypt_auth_len);
>> -       ceph_encode_32(&p, arg->fscrypt_file_len);
>> -       ceph_encode_copy(&p, arg->fscrypt_file, arg->fscrypt_file_len);
>> +       ceph_encode_32(&p, sizeof(__le64));
>> +       ceph_encode_64(&p, fc->size);
>>
>> That means no matter the 'arg->encrypted' is true or not, here it will
>> always encode extra 8 bytes' data ?
>>
>>
>> But in cap_msg_size(), you are making it optional:
>>
>>
>>    static inline int cap_msg_size(struct cap_msg_args *arg)
>>    {
>>           return CAP_MSG_FIXED_FIELDS + arg->fscrypt_auth_len +
>> -                       arg->fscrypt_file_len;
>> +                       arg->encrypted ? sizeof(__le64) : 0;
>>    }
>>
>>
>> Have I missed something important here ?
>>
>> Thanks
>>
> Nope, you're right. I had fixed that one in my local branch already, and
> just hadn't yet pushed it to the repo. I'll plan to clean this up a bit
> later today and push an updated branch.

Cool.

Thanks.


>
>> On 6/25/21 9:58 PM, Jeff Layton wrote:
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/ceph/caps.c | 62 +++++++++++++++++++++++++++++++++++++++-----------
>>>    1 file changed, 49 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>> index 038f59cc4250..1be6c5148700 100644
>>> --- a/fs/ceph/caps.c
>>> +++ b/fs/ceph/caps.c
>>> @@ -13,6 +13,7 @@
>>>    #include "super.h"
>>>    #include "mds_client.h"
>>>    #include "cache.h"
>>> +#include "crypto.h"
>>>    #include <linux/ceph/decode.h>
>>>    #include <linux/ceph/messenger.h>
>>>    
>>> @@ -1229,15 +1230,12 @@ struct cap_msg_args {
>>>    	umode_t			mode;
>>>    	bool			inline_data;
>>>    	bool			wake;
>>> +	u32			fscrypt_auth_len;
>>> +	u32			fscrypt_file_len;
>>> +	u8			fscrypt_auth[sizeof(struct ceph_fscrypt_auth)]; // for context
>>> +	u8			fscrypt_file[sizeof(u64)]; // for size
>>>    };
>>>    
>>> -/*
>>> - * cap struct size + flock buffer size + inline version + inline data size +
>>> - * osd_epoch_barrier + oldest_flush_tid
>>> - */
>>> -#define CAP_MSG_SIZE (sizeof(struct ceph_mds_caps) + \
>>> -		      4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4)
>>> -
>>>    /* Marshal up the cap msg to the MDS */
>>>    static void encode_cap_msg(struct ceph_msg *msg, struct cap_msg_args *arg)
>>>    {
>>> @@ -1253,7 +1251,7 @@ static void encode_cap_msg(struct ceph_msg *msg, struct cap_msg_args *arg)
>>>    	     arg->size, arg->max_size, arg->xattr_version,
>>>    	     arg->xattr_buf ? (int)arg->xattr_buf->vec.iov_len : 0);
>>>    
>>> -	msg->hdr.version = cpu_to_le16(10);
>>> +	msg->hdr.version = cpu_to_le16(12);
>>>    	msg->hdr.tid = cpu_to_le64(arg->flush_tid);
>>>    
>>>    	fc = msg->front.iov_base;
>>> @@ -1324,6 +1322,16 @@ static void encode_cap_msg(struct ceph_msg *msg, struct cap_msg_args *arg)
>>>    
>>>    	/* Advisory flags (version 10) */
>>>    	ceph_encode_32(&p, arg->flags);
>>> +
>>> +	/* dirstats (version 11) - these are r/o on the client */
>>> +	ceph_encode_64(&p, 0);
>>> +	ceph_encode_64(&p, 0);
>>> +
>>> +	/* fscrypt_auth and fscrypt_file (version 12) */
>>> +	ceph_encode_32(&p, arg->fscrypt_auth_len);
>>> +	ceph_encode_copy(&p, arg->fscrypt_auth, arg->fscrypt_auth_len);
>>> +	ceph_encode_32(&p, arg->fscrypt_file_len);
>>> +	ceph_encode_copy(&p, arg->fscrypt_file, arg->fscrypt_file_len);
>>>    }
>>>    
>>>    /*
>>> @@ -1445,6 +1453,26 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap,
>>>    		}
>>>    	}
>>>    	arg->flags = flags;
>>> +	if (ci->fscrypt_auth_len &&
>>> +	    WARN_ON_ONCE(ci->fscrypt_auth_len != sizeof(struct ceph_fscrypt_auth))) {
>>> +		/* Don't set this if it isn't right size */
>>> +		arg->fscrypt_auth_len = 0;
>>> +	} else {
>>> +		arg->fscrypt_auth_len = ci->fscrypt_auth_len;
>>> +		memcpy(arg->fscrypt_auth, ci->fscrypt_auth,
>>> +			min_t(size_t, ci->fscrypt_auth_len, sizeof(arg->fscrypt_auth)));
>>> +	}
>>> +	/* FIXME: use this to track "real" size */
>>> +	arg->fscrypt_file_len = 0;
>>> +}
>>> +
>>> +#define CAP_MSG_FIXED_FIELDS (sizeof(struct ceph_mds_caps) + \
>>> +		      4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4 + 8 + 8 + 4 + 4)
>>> +
>>> +static inline int cap_msg_size(struct cap_msg_args *arg)
>>> +{
>>> +	return CAP_MSG_FIXED_FIELDS + arg->fscrypt_auth_len +
>>> +			arg->fscrypt_file_len;
>>>    }
>>>    
>>>    /*
>>> @@ -1457,7 +1485,7 @@ static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci)
>>>    	struct ceph_msg *msg;
>>>    	struct inode *inode = &ci->vfs_inode;
>>>    
>>> -	msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, CAP_MSG_SIZE, GFP_NOFS, false);
>>> +	msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, cap_msg_size(arg), GFP_NOFS, false);
>>>    	if (!msg) {
>>>    		pr_err("error allocating cap msg: ino (%llx.%llx) flushing %s tid %llu, requeuing cap.\n",
>>>    		       ceph_vinop(inode), ceph_cap_string(arg->dirty),
>>> @@ -1483,10 +1511,6 @@ static inline int __send_flush_snap(struct inode *inode,
>>>    	struct cap_msg_args	arg;
>>>    	struct ceph_msg		*msg;
>>>    
>>> -	msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, CAP_MSG_SIZE, GFP_NOFS, false);
>>> -	if (!msg)
>>> -		return -ENOMEM;
>>> -
>>>    	arg.session = session;
>>>    	arg.ino = ceph_vino(inode).ino;
>>>    	arg.cid = 0;
>>> @@ -1524,6 +1548,18 @@ static inline int __send_flush_snap(struct inode *inode,
>>>    	arg.flags = 0;
>>>    	arg.wake = false;
>>>    
>>> +	/*
>>> +	 * No fscrypt_auth changes from a capsnap. It will need
>>> +	 * to update fscrypt_file on size changes (TODO).
>>> +	 */
>>> +	arg.fscrypt_auth_len = 0;
>>> +	arg.fscrypt_file_len = 0;
>>> +
>>> +	msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, cap_msg_size(&arg),
>>> +			   GFP_NOFS, false);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +
>>>    	encode_cap_msg(msg, &arg);
>>>    	ceph_con_send(&arg.session->s_con, msg);
>>>    	return 0;