From: Luis Henriques <lhenriques@suse.de>
To: Jeff Layton <jlayton@kernel.org>
Cc: ceph-devel@vger.kernel.org, linux-fscrypt@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [RFC PATCH v4 05/17] ceph: crypto context handling for ceph
Date: Mon, 25 Jan 2021 10:14:01 +0000 [thread overview]
Message-ID: <874kj5wcgm.fsf@suse.de> (raw)
In-Reply-To: <48cd711c7f99e6bd52f4ba0565eb54589843ac89.camel@kernel.org> (Jeff Layton's message of "Fri, 22 Jan 2021 12:26:38 -0500")
Jeff Layton <jlayton@kernel.org> writes:
> On Fri, 2021-01-22 at 16:41 +0000, Luis Henriques wrote:
>> Jeff Layton <jlayton@kernel.org> 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@kernel.org>
>> > ---
>> > 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
next prev parent reply other threads:[~2021-01-26 3:11 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-20 18:28 [RFC PATCH v4 00/17] ceph+fscrypt: context, filename and symlink support Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 01/17] vfs: export new_inode_pseudo Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 02/17] fscrypt: export fscrypt_base64_encode and fscrypt_base64_decode Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 03/17] fscrypt: export fscrypt_fname_encrypt and fscrypt_fname_encrypted_size Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 04/17] fscrypt: add fscrypt_context_for_new_inode Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 05/17] ceph: crypto context handling for ceph Jeff Layton
2021-01-22 16:41 ` Luis Henriques
2021-01-22 17:26 ` Jeff Layton
2021-01-25 10:14 ` Luis Henriques [this message]
2021-01-20 18:28 ` [RFC PATCH v4 06/17] ceph: implement -o test_dummy_encryption mount option Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 07/17] ceph: preallocate inode for ops that may create one Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 08/17] ceph: add routine to create fscrypt context prior to RPC Jeff Layton
2021-01-22 16:50 ` Luis Henriques
2021-01-22 17:32 ` Jeff Layton
2021-01-25 10:14 ` Luis Henriques
2021-01-20 18:28 ` [RFC PATCH v4 09/17] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 10/17] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 11/17] ceph: decode alternate_name in lease info Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 12/17] ceph: send altname in MClientRequest Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 13/17] ceph: add support to readdir for encrypted filenames Jeff Layton
2021-01-28 11:33 ` Luis Henriques
2021-01-28 13:41 ` Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 14/17] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 15/17] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
2021-02-01 17:18 ` Luis Henriques
2021-02-01 18:41 ` Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 16/17] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
2021-01-25 16:03 ` Luis Henriques
2021-01-25 18:31 ` Jeff Layton
2021-01-20 18:28 ` [RFC PATCH v4 17/17] ceph: add fscrypt ioctls Jeff Layton
2021-01-28 12:22 ` Luis Henriques
2021-01-28 13:44 ` Jeff Layton
2021-01-28 14:09 ` Luis Henriques
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874kj5wcgm.fsf@suse.de \
--to=lhenriques@suse.de \
--cc=ceph-devel@vger.kernel.org \
--cc=jlayton@kernel.org \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).