CEPH-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: ceph-devel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, Xiubo Li <xiubli@redhat.com>
Subject: Re: [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path
Date: Wed, 09 Sep 2020 08:24:39 -0400
Message-ID: <fbf3c8cea47f021200937273fc810b1244e186a1.camel@kernel.org> (raw)
In-Reply-To: <20200908050643.GL68127@sol.localdomain>

On Mon, 2020-09-07 at 22:06 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:33PM -0400, Jeff Layton wrote:
> > Allow ceph_mdsc_build_path to encrypt and base64 encode the filename
> > when the parent is encrypted and we're sending the path to the MDS.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/mds_client.c | 51 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 40 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index e3dc061252d4..26b43ae09823 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/ratelimit.h>
> >  #include <linux/bits.h>
> >  #include <linux/ktime.h>
> > +#include <linux/base64_fname.h>
> >  
> >  #include "super.h"
> >  #include "mds_client.h"
> > @@ -2324,8 +2325,7 @@ static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
> >   * Encode hidden .snap dirs as a double /, i.e.
> >   *   foo/.snap/bar -> foo//bar
> >   */
> > -char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> > -			   int stop_on_nosnap)
> > +char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire)
> >  {
> >  	struct dentry *cur;
> >  	struct inode *inode;
> > @@ -2347,30 +2347,59 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase,
> >  	seq = read_seqbegin(&rename_lock);
> >  	cur = dget(dentry);
> >  	for (;;) {
> > -		struct dentry *temp;
> > +		struct dentry *parent;
> >  
> >  		spin_lock(&cur->d_lock);
> >  		inode = d_inode(cur);
> > +		parent = cur->d_parent;
> >  		if (inode && ceph_snap(inode) == CEPH_SNAPDIR) {
> >  			dout("build_path path+%d: %p SNAPDIR\n",
> >  			     pos, cur);
> > -		} else if (stop_on_nosnap && inode && dentry != cur &&
> > -			   ceph_snap(inode) == CEPH_NOSNAP) {
> > +			dget(parent);
> > +			spin_unlock(&cur->d_lock);
> > +		} else if (for_wire && inode && dentry != cur && ceph_snap(inode) == CEPH_NOSNAP) {
> >  			spin_unlock(&cur->d_lock);
> >  			pos++; /* get rid of any prepended '/' */
> >  			break;
> > -		} else {
> > +		} else if (!for_wire || !IS_ENCRYPTED(d_inode(parent))) {
> >  			pos -= cur->d_name.len;
> >  			if (pos < 0) {
> >  				spin_unlock(&cur->d_lock);
> >  				break;
> >  			}
> >  			memcpy(path + pos, cur->d_name.name, cur->d_name.len);
> > +			dget(parent);
> > +			spin_unlock(&cur->d_lock);
> > +		} else {
> > +			int err;
> > +			struct fscrypt_name fname = { };
> > +			int len;
> > +			char buf[BASE64_CHARS(NAME_MAX)];
> > +
> > +			dget(parent);
> > +			spin_unlock(&cur->d_lock);
> > +
> > +			err = fscrypt_setup_filename(d_inode(parent), &cur->d_name, 1, &fname);
> 
> How are no-key filenames handled with ceph?  You're calling
> fscrypt_setup_filename() with lookup=1, so it will give you back a no-key
> filename if the directory's encryption key is unavailable.
> 

Still TBD.

For now, I'm just ignoring long filenames. Eventually we'll need to
extend the MDS and protocol to handle the nokey names properly and this
code will need to be reworked.

I have this bug opened for tracking that work:

    https://tracker.ceph.com/issues/47162

 
> > +			if (err) {
> > +				dput(parent);
> > +				dput(cur);
> > +				return ERR_PTR(err);
> > +			}
> > +
> > +			/* base64 encode the encrypted name */
> > +			len = base64_encode_fname(fname.disk_name.name, fname.disk_name.len, buf);
> > +			pos -= len;
> > +			if (pos < 0) {
> > +				dput(parent);
> > +				fscrypt_free_filename(&fname);
> > +				break;
> > +			}
> > +			memcpy(path + pos, buf, len);
> > +			dout("non-ciphertext name = %.*s\n", len, buf);
> > +			fscrypt_free_filename(&fname);
> 
> This would be easier to understand if the encryption and encoding logic was
> moved into its own function.
> 


Agreed, though it's a little hard given the way this function is
structured. I'll see what I can do to clean it up though.

-- 
Jeff Layton <jlayton@kernel.org>


  reply index

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04 16:05 [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 01/18] vfs: export new_inode_pseudo Jeff Layton
2020-09-08  3:38   ` Eric Biggers
2020-09-08 11:27     ` Jeff Layton
2020-09-08 22:31       ` Eric Biggers
2020-09-09 10:47         ` Jeff Layton
2020-09-09 16:12           ` Eric Biggers
2020-09-09 16:51             ` Jeff Layton
2020-09-09 18:49               ` Eric Biggers
2020-09-09 19:24                 ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 02/18] fscrypt: drop unused inode argument from fscrypt_fname_alloc_buffer Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 03/18] fscrypt: export fscrypt_d_revalidate Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 04/18] fscrypt: add fscrypt_new_context_from_inode Jeff Layton
2020-09-08  3:48   ` Eric Biggers
2020-09-08 11:29     ` Jeff Layton
2020-09-08 12:29     ` Jeff Layton
2020-09-08 22:34       ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 05/18] fscrypt: don't balk when inode is already marked encrypted Jeff Layton
2020-09-08  3:52   ` Eric Biggers
2020-09-08 12:54     ` Jeff Layton
2020-09-08 23:08       ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 06/18] fscrypt: move nokey_name conversion to separate function and export it Jeff Layton
2020-09-08  3:55   ` Eric Biggers
2020-09-08 12:50     ` Jeff Layton
2020-09-08 22:53       ` Eric Biggers
2020-09-09 16:02         ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 07/18] lib: lift fscrypt base64 conversion into lib/ Jeff Layton
2020-09-08  3:59   ` Eric Biggers
2020-09-08 12:51     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 08/18] ceph: add fscrypt ioctls Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 09/18] ceph: crypto context handling for ceph Jeff Layton
2020-09-08  4:29   ` Eric Biggers
2020-09-08 16:14     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 10/18] ceph: preallocate inode for ops that may create one Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 11/18] ceph: add routine to create context prior to RPC Jeff Layton
2020-09-08  4:43   ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 12/18] ceph: set S_ENCRYPTED bit if new inode has encryption.ctx xattr Jeff Layton
2020-09-08  4:57   ` Eric Biggers
2020-09-09 12:20     ` Jeff Layton
2020-09-09 15:53     ` Jeff Layton
2020-09-09 16:33       ` Eric Biggers
2020-09-09 17:19         ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 13/18] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
2020-09-08  5:06   ` Eric Biggers
2020-09-09 12:24     ` Jeff Layton [this message]
2020-09-04 16:05 ` [RFC PATCH v2 15/18] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
2020-09-08  5:12   ` Eric Biggers
2020-09-09 12:26     ` Jeff Layton
2020-09-09 16:18       ` Eric Biggers
2020-09-04 16:05 ` [RFC PATCH v2 16/18] ceph: add support to readdir for encrypted filenames Jeff Layton
2020-09-08  5:34   ` Eric Biggers
2020-09-09 13:02     ` Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 17/18] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
2020-09-04 16:05 ` [RFC PATCH v2 18/18] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
2020-09-04 16:11   ` Jeff Layton
2020-09-08  5:43   ` Eric Biggers
2020-09-08  5:54 ` [RFC PATCH v2 00/18] ceph+fscrypt: context, filename and symlink support Eric Biggers
2020-09-08 12:09   ` Jeff Layton

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=fbf3c8cea47f021200937273fc810b1244e186a1.camel@kernel.org \
    --to=jlayton@kernel.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=ebiggers@kernel.org \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=xiubli@redhat.com \
    /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

CEPH-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/ceph-devel/0 ceph-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ceph-devel ceph-devel/ https://lore.kernel.org/ceph-devel \
		ceph-devel@vger.kernel.org
	public-inbox-index ceph-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.ceph-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git