Re: [RFC PATCH v2 14/18] ceph: add encrypted fname handling to ceph_mdsc_build_path

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	[thread overview]
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>