diff mbox series

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

Message ID 20200904160537.76663-15-jlayton@kernel.org
State Superseded
Headers show
Series ceph+fscrypt: context, filename and symlink support | expand

Commit Message

Jeff Layton Sept. 4, 2020, 4:05 p.m. UTC
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(-)

Comments

Eric Biggers Sept. 8, 2020, 5:06 a.m. UTC | #1
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.

> +			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.

- Eric
Jeff Layton Sept. 9, 2020, 12:24 p.m. UTC | #2
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.
diff mbox series

Patch

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);
+			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);
 		}
-		temp = cur;
-		cur = dget(temp->d_parent);
-		spin_unlock(&temp->d_lock);
-		dput(temp);
+		dput(cur);
+		cur = parent;
 
 		/* Are we at the root? */
 		if (IS_ROOT(cur))
@@ -2415,7 +2444,7 @@  static int build_dentry_path(struct dentry *dentry, struct inode *dir,
 	rcu_read_lock();
 	if (!dir)
 		dir = d_inode_rcu(dentry->d_parent);
-	if (dir && parent_locked && ceph_snap(dir) == CEPH_NOSNAP) {
+	if (dir && parent_locked && ceph_snap(dir) == CEPH_NOSNAP && !IS_ENCRYPTED(dir)) {
 		*pino = ceph_ino(dir);
 		rcu_read_unlock();
 		*ppath = dentry->d_name.name;