| Message ID | 20210625135834.12934-8-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ceph+fscrypt: context, filename and symlink support | expand |
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.
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
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;
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. > 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. > > 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; >
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;
On Fri, Jun 25, 2021 at 09:58:17AM -0400, 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 > }; The naming of these is confusing to me. If these are the fscrypt context and the original file size, why aren't they called something like fscrypt_context and fscrypt_file_size? Also does the file size really need to be variable-length, or could it just be a 64-bit integer? - Eric
On Sun, 2021-07-11 at 18:00 -0500, Eric Biggers wrote: > On Fri, Jun 25, 2021 at 09:58:17AM -0400, 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 > > }; > > The naming of these is confusing to me. If these are the fscrypt context and > the original file size, why aren't they called something like fscrypt_context > and fscrypt_file_size? > > Also does the file size really need to be variable-length, or could it just be a > 64-bit integer? > Fscrypt is really a kernel client-side feature. Both of these new fields are treated as opaque by the MDS and are wholly managed by the client. We need two fields because they are governed by different cephfs capabilities (aka "caps"). AUTH caps for the context and FILE caps for the size. So we have two new fields -- fscrypt_file and fscrypt_auth. The size could be a __le64 or something, but I think it makes sense to allow it to be opaque as we aren't certain what other info we might want to keep in there. We might also want to encrypt the fscrypt_file field to cloak the true size of a file from anyone without the key. Now, all that said, the fact that the MDS largely handles truncation poses some special challenges for the content encryption piece. We may ultimately end up making this more special-purpose than it is now.
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;
Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/caps.c | 62 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 13 deletions(-)