Jeff Layton <jlayton@xxxxxxxxxx> writes:
> On Fri, 2021-01-22 at 16:41 +0000, Luis Henriques wrote:
>> Jeff Layton <jlayton@xxxxxxxxxx> writes:
>>
>> > Store the fscrypt context for an inode as an encryption.ctx xattr.
>> > When we get a new inode in a trace, set the S_ENCRYPTED bit if
>> > the xattr blob has an encryption.ctx xattr.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> > ---
>> > fs/ceph/Makefile | 1 +
>> > fs/ceph/crypto.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>> > fs/ceph/crypto.h | 24 ++++++++++++++++++++++++
>> > fs/ceph/inode.c | 6 ++++++
>> > fs/ceph/super.c | 3 +++
>> > fs/ceph/super.h | 1 +
>> > fs/ceph/xattr.c | 32 ++++++++++++++++++++++++++++++++
>> > 7 files changed, 109 insertions(+)
>> > create mode 100644 fs/ceph/crypto.c
>> > create mode 100644 fs/ceph/crypto.h
>> >
>> > diff --git a/fs/ceph/Makefile b/fs/ceph/Makefile
>> > index 50c635dc7f71..1f77ca04c426 100644
>> > --- a/fs/ceph/Makefile
>> > +++ b/fs/ceph/Makefile
>> > @@ -12,3 +12,4 @@ ceph-y := super.o inode.o dir.o file.o locks.o addr.o ioctl.o \
>> >
>> >
>> >
>> >
>> > ceph-$(CONFIG_CEPH_FSCACHE) += cache.o
>> > ceph-$(CONFIG_CEPH_FS_POSIX_ACL) += acl.o
>> > +ceph-$(CONFIG_FS_ENCRYPTION) += crypto.o
>> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> > new file mode 100644
>> > index 000000000000..dbe8b60fd1b0
>> > --- /dev/null
>> > +++ b/fs/ceph/crypto.c
>> > @@ -0,0 +1,42 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +#include <linux/ceph/ceph_debug.h>
>> > +#include <linux/xattr.h>
>> > +#include <linux/fscrypt.h>
>> > +
>> > +#include "super.h"
>> > +#include "crypto.h"
>> > +
>> > +static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>> > +{
>> > + return __ceph_getxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len);
>> > +}
>> > +
>> > +static int ceph_crypt_set_context(struct inode *inode, const void *ctx, size_t len, void *fs_data)
>> > +{
>> > + int ret;
>> > +
>> > + WARN_ON_ONCE(fs_data);
>> > + ret = __ceph_setxattr(inode, CEPH_XATTR_NAME_ENCRYPTION_CONTEXT, ctx, len, XATTR_CREATE);
>> > + if (ret == 0)
>> > + inode_set_flags(inode, S_ENCRYPTED, S_ENCRYPTED);
>> > + return ret;
>> > +}
>> > +
>> > +static bool ceph_crypt_empty_dir(struct inode *inode)
>> > +{
>> > + struct ceph_inode_info *ci = ceph_inode(inode);
>> > +
>> > + return ci->i_rsubdirs + ci->i_rfiles == 1;
>> > +}
>>
>> This is very tricky, as this check can't really guaranty that the
>> directory is empty. We need to make sure no other client has access to
>> this directory during the whole operation of setting policy. Would it be
>> enough to ensure we have Fxc here?
>>
>
> That's a good point. Yes, we probably should do just that. I'll take a
> look and see what we need as far as caps go to ensure exclusion.
>
> empty_dir is only called when setting the context, so we can grab cap
> refs at that point and then just check to make sure it's empty here.
>
>> > +
...
>> > +static struct fscrypt_operations ceph_fscrypt_ops = {
>> > +/* Return true if inode's xattr blob has an xattr named "name" */
>> > +bool ceph_inode_has_xattr(struct ceph_inode_info *ci, const char *name)
>> > +{
>> > + void *p, *end;
>> > + u32 numattr;
>> > + size_t namelen;
>> > +
>> > + lockdep_assert_held(&ci->i_ceph_lock);
>> > +
>> > + if (!ci->i_xattrs.blob || ci->i_xattrs.blob->vec.iov_len <= 4)
>> > + return false;
>> > +
>> > + namelen = strlen(name);
>> > + p = ci->i_xattrs.blob->vec.iov_base;
>> > + end = p + ci->i_xattrs.blob->vec.iov_len;
>> > + ceph_decode_32_safe(&p, end, numattr, bad);
>> > +
>> > + while (numattr--) {
>> > + u32 len;
>> > +
>> > + ceph_decode_32_safe(&p, end, len, bad);
>> > + ceph_decode_need(&p, end, len, bad);
>> > + if (len == namelen && !memcmp(p, name, len))
>> > + return true;
>> > + p += len;
>> > + ceph_decode_32_safe(&p, end, len, bad);
>> > + ceph_decode_skip_n(&p, end, len, bad);
>> > + }
>> > +bad:
>> > + return false;
>> > +}
>>
>> I wonder if it wouldn't be better have an extra flag in struct
>> ceph_inode_info instead of having to go through the xattr list every time
>> we update an inode with data from the MDS.
>>
>
> It is ugly, I'll grant you that. A flag in ceph_inode_info won't really
> help though. We'd need some sort of flag in the inode info transmitted
> from the MDS.
>
> For now, we only call this for I_NEW inodes, but now I'm wondering if we
> need to check it every time, to ensure that everything works if you see
> the directory before another client encrypts it.
>
> We may want to extend the protocol with such a flag, but the xattr
> buffer is not usually that long. For now, I'm inclined to live with
> parsing this info each time.
Ok, makes sense. Thanks for clarifying. I guess that getting the Fxc
caps when encrypting a dir (setting the context) may actually prevent the
race you mention anyway.
Cheers,
--
Luis
