[PATCH v2 0/7] ceph: encrypt the snapshot directories

[PATCH v2 0/7] ceph: encrypt the snapshot directories

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

This patch series is base the 'wip-fscrypt' branch in ceph-client.

V2:
- Fix several bugs, such as for the long snap name encrypt/dencrypt
- Skip double dencypting dentry names for readdir

======

NOTE: This patch series won't fix the long snap shot issue as Luis
is working on that.


Xiubo Li (7):
  ceph: fail the request when failing to decode dentry names
  ceph: skip the memories when received a higher version of message
  ceph: do not dencrypt the dentry name twice for readdir
  ceph: add ceph_get_snap_parent_inode() support
  ceph: use the parent inode of '.snap' to dencrypt the names for
    readdir
  ceph: use the parent inode of '.snap' to encrypt name to build path
  ceph: try to encrypt/decrypt long snap name

 fs/ceph/crypto.c     |  75 ++++++++++++++++++++++++++---
 fs/ceph/crypto.h     |   2 +-
 fs/ceph/dir.c        |  87 +++++++++++++++++++---------------
 fs/ceph/inode.c      | 110 ++++++++++++++++++++++++++++++++++++++-----
 fs/ceph/mds_client.c |  59 ++++++++++++++---------
 fs/ceph/mds_client.h |   3 ++
 fs/ceph/snap.c       |  24 ++++++++++
 fs/ceph/super.h      |   2 +
 8 files changed, 286 insertions(+), 76 deletions(-)

-- 
2.27.0

[PATCH v2 1/7] ceph: fail the request when failing to decode dentry names

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

------------[ cut here ]------------
kernel BUG at fs/ceph/dir.c:537!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
CPU: 16 PID: 21641 Comm: ls Tainted: G            E     5.17.0-rc2+ #92
Hardware name: Red Hat RHEV Hypervisor, BIOS 1.11.0-2.el7 04/01/2014

The corresponding code in ceph_readdir() is:

	BUG_ON(rde->offset < ctx->pos);

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/dir.c        | 13 +++++++------
 fs/ceph/inode.c      |  5 +++--
 fs/ceph/mds_client.c |  2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index a449f4a07c07..6be0c1f793c2 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -534,6 +534,13 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 					    .ctext_len	= rde->altname_len };
 		u32 olen = oname.len;
 
+		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
+		if (err) {
+			pr_err("%s unable to decode %.*s, got %d\n", __func__,
+			       rde->name_len, rde->name, err);
+			goto out;
+		}
+
 		BUG_ON(rde->offset < ctx->pos);
 		BUG_ON(!rde->inode.in);
 
@@ -542,12 +549,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		     i, rinfo->dir_nr, ctx->pos,
 		     rde->name_len, rde->name, &rde->inode.in);
 
-		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
-		if (err) {
-			dout("Unable to decode %.*s. Skipping it.\n", rde->name_len, rde->name);
-			continue;
-		}
-
 		if (!dir_emit(ctx, oname.name, oname.len,
 			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
 			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8b0832271fdf..2bc2f02b84e8 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1898,8 +1898,9 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 
 		err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
 		if (err) {
-			dout("Unable to decode %.*s. Skipping it.", rde->name_len, rde->name);
-			continue;
+			pr_err("%s unable to decode %.*s, got %d\n", __func__,
+			       rde->name_len, rde->name, err);
+			goto out;
 		}
 
 		dname.name = oname.name;
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 914a6e68bb56..94b4c6508044 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3474,7 +3474,7 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
 	if (err == 0) {
 		if (result == 0 && (req->r_op == CEPH_MDS_OP_READDIR ||
 				    req->r_op == CEPH_MDS_OP_LSSNAP))
-			ceph_readdir_prepopulate(req, req->r_session);
+			err = ceph_readdir_prepopulate(req, req->r_session);
 	}
 	current->journal_info = NULL;
 	mutex_unlock(&req->r_fill_mutex);
-- 
2.27.0

Re: [PATCH v2 1/7] ceph: fail the request when failing to decode dentry names

[Jeff Layton]

On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> ------------[ cut here ]------------
> kernel BUG at fs/ceph/dir.c:537!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
> CPU: 16 PID: 21641 Comm: ls Tainted: G            E     5.17.0-rc2+ #92
> Hardware name: Red Hat RHEV Hypervisor, BIOS 1.11.0-2.el7 04/01/2014
> 
> The corresponding code in ceph_readdir() is:
> 
> 	BUG_ON(rde->offset < ctx->pos);
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/dir.c        | 13 +++++++------
>  fs/ceph/inode.c      |  5 +++--
>  fs/ceph/mds_client.c |  2 +-
>  3 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index a449f4a07c07..6be0c1f793c2 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -534,6 +534,13 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  					    .ctext_len	= rde->altname_len };
>  		u32 olen = oname.len;
>  
> +		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> +		if (err) {
> +			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> +			       rde->name_len, rde->name, err);
> +			goto out;
> +		}
> +
>  		BUG_ON(rde->offset < ctx->pos);
>  		BUG_ON(!rde->inode.in);
>  
> @@ -542,12 +549,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  		     i, rinfo->dir_nr, ctx->pos,
>  		     rde->name_len, rde->name, &rde->inode.in);
>  
> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> -		if (err) {
> -			dout("Unable to decode %.*s. Skipping it.\n", rde->name_len, rde->name);
> -			continue;
> -		}
> -
>  		if (!dir_emit(ctx, oname.name, oname.len,
>  			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>  			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 8b0832271fdf..2bc2f02b84e8 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1898,8 +1898,9 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>  
>  		err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
>  		if (err) {
> -			dout("Unable to decode %.*s. Skipping it.", rde->name_len, rde->name);
> -			continue;
> +			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> +			       rde->name_len, rde->name, err);
> +			goto out;
>  		}
>  


Is this really an improvement? Suppose I have one dentry with a corrupt
name. Do I want to fail a readdir request which might allow me to get at
other dentries in that directory that isn't corrupt?

Maybe we should try to emit some placeholder there?


>  		dname.name = oname.name;
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 914a6e68bb56..94b4c6508044 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3474,7 +3474,7 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>  	if (err == 0) {
>  		if (result == 0 && (req->r_op == CEPH_MDS_OP_READDIR ||
>  				    req->r_op == CEPH_MDS_OP_LSSNAP))
> -			ceph_readdir_prepopulate(req, req->r_session);
> +			err = ceph_readdir_prepopulate(req, req->r_session);
>  	}
>  	current->journal_info = NULL;
>  	mutex_unlock(&req->r_fill_mutex);

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 1/7] ceph: fail the request when failing to decode dentry names

[Xiubo Li]

On 3/1/22 9:20 PM, Jeff Layton wrote:
> On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> ------------[ cut here ]------------
>> kernel BUG at fs/ceph/dir.c:537!
>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
>> CPU: 16 PID: 21641 Comm: ls Tainted: G            E     5.17.0-rc2+ #92
>> Hardware name: Red Hat RHEV Hypervisor, BIOS 1.11.0-2.el7 04/01/2014
>>
>> The corresponding code in ceph_readdir() is:
>>
>> 	BUG_ON(rde->offset < ctx->pos);
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/dir.c        | 13 +++++++------
>>   fs/ceph/inode.c      |  5 +++--
>>   fs/ceph/mds_client.c |  2 +-
>>   3 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index a449f4a07c07..6be0c1f793c2 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -534,6 +534,13 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   					    .ctext_len	= rde->altname_len };
>>   		u32 olen = oname.len;
>>   
>> +		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>> +		if (err) {
>> +			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>> +			       rde->name_len, rde->name, err);
>> +			goto out;
>> +		}
>> +
>>   		BUG_ON(rde->offset < ctx->pos);
>>   		BUG_ON(!rde->inode.in);
>>   
>> @@ -542,12 +549,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   		     i, rinfo->dir_nr, ctx->pos,
>>   		     rde->name_len, rde->name, &rde->inode.in);
>>   
>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>> -		if (err) {
>> -			dout("Unable to decode %.*s. Skipping it.\n", rde->name_len, rde->name);
>> -			continue;
>> -		}
>> -
>>   		if (!dir_emit(ctx, oname.name, oname.len,
>>   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 8b0832271fdf..2bc2f02b84e8 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -1898,8 +1898,9 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>   
>>   		err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
>>   		if (err) {
>> -			dout("Unable to decode %.*s. Skipping it.", rde->name_len, rde->name);
>> -			continue;
>> +			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>> +			       rde->name_len, rde->name, err);
>> +			goto out;
>>   		}
>>   
>
> Is this really an improvement?

Yeah, if we just continue without setting the rde->offset it will crash 
in "BUG_ON(rde->offset < ctx->pos);" in ceph_readdir().


> Suppose I have one dentry with a corrupt
> name. Do I want to fail a readdir request which might allow me to get at
> other dentries in that directory that isn't corrupt?

It's a little hard to handle the code in ceph_readdir():

  503         /* search start position */
  504         if (rinfo->dir_nr > 0) {
  505                 int step, nr = rinfo->dir_nr;
  506                 while (nr > 0) {
  507                         step = nr >> 1;
  508                         if (rinfo->dir_entries[i + step].offset < 
ctx->pos) {
  509                                 i +=  step + 1;
  510                                 nr -= step + 1;
  511                         } else {
  512                                 nr = step;
  513                         }
  514                 }
  515         }

In this case how to set the rde->offset ?


>
> Maybe we should try to emit some placeholder there?
>
>
>>   		dname.name = oname.name;
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index 914a6e68bb56..94b4c6508044 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -3474,7 +3474,7 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>>   	if (err == 0) {
>>   		if (result == 0 && (req->r_op == CEPH_MDS_OP_READDIR ||
>>   				    req->r_op == CEPH_MDS_OP_LSSNAP))
>> -			ceph_readdir_prepopulate(req, req->r_session);
>> +			err = ceph_readdir_prepopulate(req, req->r_session);
>>   	}
>>   	current->journal_info = NULL;
>>   	mutex_unlock(&req->r_fill_mutex);

Re: [PATCH v2 1/7] ceph: fail the request when failing to decode dentry names

[Jeff Layton]

On Tue, 2022-03-01 at 21:57 +0800, Xiubo Li wrote:
> On 3/1/22 9:20 PM, Jeff Layton wrote:
> > On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
> > > From: Xiubo Li <xiubli@redhat.com>
> > > 
> > > ------------[ cut here ]------------
> > > kernel BUG at fs/ceph/dir.c:537!
> > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
> > > CPU: 16 PID: 21641 Comm: ls Tainted: G            E     5.17.0-rc2+ #92
> > > Hardware name: Red Hat RHEV Hypervisor, BIOS 1.11.0-2.el7 04/01/2014
> > > 
> > > The corresponding code in ceph_readdir() is:
> > > 
> > > 	BUG_ON(rde->offset < ctx->pos);
> > > 
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > ---
> > >   fs/ceph/dir.c        | 13 +++++++------
> > >   fs/ceph/inode.c      |  5 +++--
> > >   fs/ceph/mds_client.c |  2 +-
> > >   3 files changed, 11 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > index a449f4a07c07..6be0c1f793c2 100644
> > > --- a/fs/ceph/dir.c
> > > +++ b/fs/ceph/dir.c
> > > @@ -534,6 +534,13 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   					    .ctext_len	= rde->altname_len };
> > >   		u32 olen = oname.len;
> > >   
> > > +		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > +		if (err) {
> > > +			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > +			       rde->name_len, rde->name, err);
> > > +			goto out;
> > > +		}
> > > +
> > >   		BUG_ON(rde->offset < ctx->pos);
> > >   		BUG_ON(!rde->inode.in);
> > >   
> > > @@ -542,12 +549,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   		     i, rinfo->dir_nr, ctx->pos,
> > >   		     rde->name_len, rde->name, &rde->inode.in);
> > >   
> > > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > -		if (err) {
> > > -			dout("Unable to decode %.*s. Skipping it.\n", rde->name_len, rde->name);
> > > -			continue;
> > > -		}
> > > -
> > >   		if (!dir_emit(ctx, oname.name, oname.len,
> > >   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> > >   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 8b0832271fdf..2bc2f02b84e8 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -1898,8 +1898,9 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > >   
> > >   		err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
> > >   		if (err) {
> > > -			dout("Unable to decode %.*s. Skipping it.", rde->name_len, rde->name);
> > > -			continue;
> > > +			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > +			       rde->name_len, rde->name, err);
> > > +			goto out;
> > >   		}
> > >   
> > 
> > Is this really an improvement?
> 
> Yeah, if we just continue without setting the rde->offset it will crash 
> in "BUG_ON(rde->offset < ctx->pos);" in ceph_readdir().
> 
> 

Ok.

> > Suppose I have one dentry with a corrupt
> > name. Do I want to fail a readdir request which might allow me to get at
> > other dentries in that directory that isn't corrupt?
> 
> It's a little hard to handle the code in ceph_readdir():
> 
>   503         /* search start position */
>   504         if (rinfo->dir_nr > 0) {
>   505                 int step, nr = rinfo->dir_nr;
>   506                 while (nr > 0) {
>   507                         step = nr >> 1;
>   508                         if (rinfo->dir_entries[i + step].offset < 
> ctx->pos) {
>   509                                 i +=  step + 1;
>   510                                 nr -= step + 1;
>   511                         } else {
>   512                                 nr = step;
>   513                         }
>   514                 }
>   515         }
> 
> In this case how to set the rde->offset ?
> 
> 

Yeah, that is the nasty part. I would probably just pretend that the
corrupt dentry doesn't exist. Offsets are set by ceph_make_fpos, AFAICT,
and I don't think skipping one should affect the position of the other.

OTOH, this is just a nice-to-have thing. If it's too nasty to deal with,
we can just return an error for now and aim to do better error handling
here later.


> > 
> > Maybe we should try to emit some placeholder there?
> > 
> > 
> > >   		dname.name = oname.name;
> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > index 914a6e68bb56..94b4c6508044 100644
> > > --- a/fs/ceph/mds_client.c
> > > +++ b/fs/ceph/mds_client.c
> > > @@ -3474,7 +3474,7 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
> > >   	if (err == 0) {
> > >   		if (result == 0 && (req->r_op == CEPH_MDS_OP_READDIR ||
> > >   				    req->r_op == CEPH_MDS_OP_LSSNAP))
> > > -			ceph_readdir_prepopulate(req, req->r_session);
> > > +			err = ceph_readdir_prepopulate(req, req->r_session);
> > >   	}
> > >   	current->journal_info = NULL;
> > >   	mutex_unlock(&req->r_fill_mutex);
> 

-- 
Jeff Layton <jlayton@kernel.org>

[PATCH v2 2/7] ceph: skip the memories when received a higher version of message

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

We should skip the extra memories which are from the higher version
just likes the libcephfs client does.

URL: https://tracker.ceph.com/issues/54430
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/mds_client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 94b4c6508044..3dea96df4769 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -326,6 +326,7 @@ static int parse_reply_info_lease(void **p, void *end,
 			goto bad;
 
 		ceph_decode_32_safe(p, end, struct_len, bad);
+		end = *p + struct_len;
 	} else {
 		struct_len = sizeof(**lease);
 		*altname_len = 0;
@@ -346,6 +347,7 @@ static int parse_reply_info_lease(void **p, void *end,
 			*altname = NULL;
 			*altname_len = 0;
 		}
+		*p = end;
 	}
 	return 0;
 bad:
-- 
2.27.0

[PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
 fs/ceph/inode.c      | 15 ++++++++++
 fs/ceph/mds_client.h |  1 +
 3 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 6be0c1f793c2..e3917b4426e8 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 	int err;
 	unsigned frag = -1;
 	struct ceph_mds_reply_info_parsed *rinfo;
-	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
-	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
+	char *dentry_name = NULL;
 
 	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
 	if (dfi->file_info.flags & CEPH_F_ATEND)
@@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		ctx->pos = 2;
 	}
 
-	err = fscrypt_prepare_readdir(inode);
-	if (err)
-		goto out;
-
 	spin_lock(&ci->i_ceph_lock);
 	/* request Fx cap. if have Fx, we don't need to release Fs cap
 	 * for later create/unlink. */
@@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 		spin_unlock(&ci->i_ceph_lock);
 	}
 
-	err = ceph_fname_alloc_buffer(inode, &tname);
-	if (err < 0)
-		goto out;
-
-	err = ceph_fname_alloc_buffer(inode, &oname);
-	if (err < 0)
-		goto out;
-
 	/* proceed with a normal readdir */
 more:
 	/* do we have the correct frag content buffered? */
@@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			}
 		}
 	}
+
+	dentry_name = kmalloc(280, GFP_KERNEL);
+	if (!dentry_name) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	for (; i < rinfo->dir_nr; i++) {
 		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
-		struct ceph_fname fname = { .dir	= inode,
-					    .name	= rde->name,
-					    .name_len	= rde->name_len,
-					    .ctext	= rde->altname,
-					    .ctext_len	= rde->altname_len };
-		u32 olen = oname.len;
-
-		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
-		if (err) {
-			pr_err("%s unable to decode %.*s, got %d\n", __func__,
-			       rde->name_len, rde->name, err);
-			goto out;
-		}
+		struct dentry *dn = rde->dentry;
+		int name_len;
 
 		BUG_ON(rde->offset < ctx->pos);
 		BUG_ON(!rde->inode.in);
+		BUG_ON(!rde->dentry);
 
 		ctx->pos = rde->offset;
-		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
-		     i, rinfo->dir_nr, ctx->pos,
-		     rde->name_len, rde->name, &rde->inode.in);
 
-		if (!dir_emit(ctx, oname.name, oname.len,
+		spin_lock(&dn->d_lock);
+		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
+		name_len = dn->d_name.len;
+		spin_unlock(&dn->d_lock);
+
+		dentry_name[name_len] = '\0';
+		dout("readdir (%d/%d) -> %llx '%s' %p\n",
+		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
+
+		dput(dn);
+		rde->dentry = NULL;
+
+		if (!dir_emit(ctx, dentry_name, name_len,
 			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
 			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
 			dout("filldir stopping us...\n");
 			err = 0;
+			for (; i < rinfo->dir_nr; i++) {
+				rde = rinfo->dir_entries + i;
+				dput(rde->dentry);
+				rde->dentry = NULL;
+			}
 			goto out;
 		}
 
-		/* Reset the lengths to their original allocated vals */
-		oname.len = olen;
 		ctx->pos++;
 	}
 
@@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 	err = 0;
 	dout("readdir %p file %p done.\n", inode, file);
 out:
-	ceph_fname_free_buffer(inode, &tname);
-	ceph_fname_free_buffer(inode, &oname);
+	if (dentry_name)
+		kfree(dentry_name);
 	return err;
 }
 
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 2bc2f02b84e8..877e699fe43b 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 			goto out;
 		}
 
+		rde->dentry = NULL;
 		dname.name = oname.name;
 		dname.len = oname.len;
 		dname.hash = full_name_hash(parent, dname.name, dname.len);
@@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 			goto retry_lookup;
 		}
 
+		/*
+		 * ceph_readdir will use the dentry to get the name
+		 * to avoid doing the dencrypt again there.
+		 */
+		rde->dentry = dget(dn);
+
 		/* inode */
 		if (d_really_is_positive(dn)) {
 			in = d_inode(dn);
@@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		dput(dn);
 	}
 out:
+	if (err) {
+		for (; i >= 0; i--) {
+			struct ceph_mds_reply_dir_entry *rde;
+
+			rde = rinfo->dir_entries + i;
+			dput(rde->dentry);
+		}
+	}
 	if (err == 0 && skipped == 0) {
 		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
 		req->r_readdir_cache_idx = cache_ctl.index;
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 0dfe24f94567..663d7754d57d 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
 };
 
 struct ceph_mds_reply_dir_entry {
+	struct dentry		      *dentry;
 	char                          *name;
 	u8			      *altname;
 	u32                           name_len;
-- 
2.27.0

Re: [PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[Jeff Layton]

On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
>  fs/ceph/inode.c      | 15 ++++++++++
>  fs/ceph/mds_client.h |  1 +
>  3 files changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6be0c1f793c2..e3917b4426e8 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  	int err;
>  	unsigned frag = -1;
>  	struct ceph_mds_reply_info_parsed *rinfo;
> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> +	char *dentry_name = NULL;
>  
>  	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>  	if (dfi->file_info.flags & CEPH_F_ATEND)
> @@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  		ctx->pos = 2;
>  	}
>  
> -	err = fscrypt_prepare_readdir(inode);
> -	if (err)
> -		goto out;
> -


Why are you removing this? This is what ensures that the key is loaded
if we're going to need it.

>  	spin_lock(&ci->i_ceph_lock);
>  	/* request Fx cap. if have Fx, we don't need to release Fs cap
>  	 * for later create/unlink. */
> @@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  		spin_unlock(&ci->i_ceph_lock);
>  	}
>  
> -	err = ceph_fname_alloc_buffer(inode, &tname);
> -	if (err < 0)
> -		goto out;
> -
> -	err = ceph_fname_alloc_buffer(inode, &oname);
> -	if (err < 0)
> -		goto out;
> -
>  	/* proceed with a normal readdir */
>  more:
>  	/* do we have the correct frag content buffered? */
> @@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  			}
>  		}
>  	}
> +
> +	dentry_name = kmalloc(280, GFP_KERNEL);
> +	if (!dentry_name) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +

Woah, what's up with the bare "280" here?


>  	for (; i < rinfo->dir_nr; i++) {
>  		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> -		struct ceph_fname fname = { .dir	= inode,
> -					    .name	= rde->name,
> -					    .name_len	= rde->name_len,
> -					    .ctext	= rde->altname,
> -					    .ctext_len	= rde->altname_len };
> -		u32 olen = oname.len;
> -
> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> -		if (err) {
> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> -			       rde->name_len, rde->name, err);
> -			goto out;
> -		}

I may be missing something, but if you rip this out, where does the
decryption happen?

> +		struct dentry *dn = rde->dentry;
> +		int name_len;
>  
>  		BUG_ON(rde->offset < ctx->pos);
>  		BUG_ON(!rde->inode.in);
> +		BUG_ON(!rde->dentry);
>  
>  		ctx->pos = rde->offset;
> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> -		     i, rinfo->dir_nr, ctx->pos,
> -		     rde->name_len, rde->name, &rde->inode.in);
>  
> -		if (!dir_emit(ctx, oname.name, oname.len,
> +		spin_lock(&dn->d_lock);
> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> +		name_len = dn->d_name.len;
> +		spin_unlock(&dn->d_lock);
> +
> +		dentry_name[name_len] = '\0';
> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> +
> +		dput(dn);
> +		rde->dentry = NULL;
> +
> +		if (!dir_emit(ctx, dentry_name, name_len,
>  			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>  			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>  			dout("filldir stopping us...\n");
>  			err = 0;
> +			for (; i < rinfo->dir_nr; i++) {
> +				rde = rinfo->dir_entries + i;
> +				dput(rde->dentry);
> +				rde->dentry = NULL;
> +			}
>  			goto out;
>  		}
>  
> -		/* Reset the lengths to their original allocated vals */
> -		oname.len = olen;
>  		ctx->pos++;
>  	}
>  
> @@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  	err = 0;
>  	dout("readdir %p file %p done.\n", inode, file);
>  out:
> -	ceph_fname_free_buffer(inode, &tname);
> -	ceph_fname_free_buffer(inode, &oname);
> +	if (dentry_name)
> +		kfree(dentry_name);
>  	return err;
>  }
>  
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 2bc2f02b84e8..877e699fe43b 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>  			goto out;
>  		}
>  
> +		rde->dentry = NULL;
>  		dname.name = oname.name;
>  		dname.len = oname.len;
>  		dname.hash = full_name_hash(parent, dname.name, dname.len);
> @@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>  			goto retry_lookup;
>  		}
>  
> +		/*
> +		 * ceph_readdir will use the dentry to get the name
> +		 * to avoid doing the dencrypt again there.
> +		 */
> +		rde->dentry = dget(dn);
> +
>  		/* inode */
>  		if (d_really_is_positive(dn)) {
>  			in = d_inode(dn);
> @@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>  		dput(dn);
>  	}
>  out:
> +	if (err) {
> +		for (; i >= 0; i--) {
> +			struct ceph_mds_reply_dir_entry *rde;
> +
> +			rde = rinfo->dir_entries + i;
> +			dput(rde->dentry);
> +		}
> +	}
>  	if (err == 0 && skipped == 0) {
>  		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
>  		req->r_readdir_cache_idx = cache_ctl.index;
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 0dfe24f94567..663d7754d57d 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>  };
>  
>  struct ceph_mds_reply_dir_entry {
> +	struct dentry		      *dentry;
>  	char                          *name;
>  	u8			      *altname;
>  	u32                           name_len;


NAK on this patch as it is...

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[Xiubo Li]

On 3/1/22 9:31 PM, Jeff Layton wrote:
> On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
>>   fs/ceph/inode.c      | 15 ++++++++++
>>   fs/ceph/mds_client.h |  1 +
>>   3 files changed, 47 insertions(+), 35 deletions(-)
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 6be0c1f793c2..e3917b4426e8 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   	int err;
>>   	unsigned frag = -1;
>>   	struct ceph_mds_reply_info_parsed *rinfo;
>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>> +	char *dentry_name = NULL;
>>   
>>   	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>   	if (dfi->file_info.flags & CEPH_F_ATEND)
>> @@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   		ctx->pos = 2;
>>   	}
>>   
>> -	err = fscrypt_prepare_readdir(inode);
>> -	if (err)
>> -		goto out;
>> -
>
> Why are you removing this? This is what ensures that the key is loaded
> if we're going to need it.

Since the dentry names are already decrypted in 
ceph_readdir_prepopulate(), and this patch is trying to avoid do the 
same thing again here after that.

Because in ceph_readdir() this isn't needed any more. And in other 
places such as build path it will do it when needed.

>
>>   	spin_lock(&ci->i_ceph_lock);
>>   	/* request Fx cap. if have Fx, we don't need to release Fs cap
>>   	 * for later create/unlink. */
>> @@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   		spin_unlock(&ci->i_ceph_lock);
>>   	}
>>   
>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>> -	if (err < 0)
>> -		goto out;
>> -
>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>> -	if (err < 0)
>> -		goto out;
>> -
>>   	/* proceed with a normal readdir */
>>   more:
>>   	/* do we have the correct frag content buffered? */
>> @@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   			}
>>   		}
>>   	}
>> +
>> +	dentry_name = kmalloc(280, GFP_KERNEL);
>> +	if (!dentry_name) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
> Woah, what's up with the bare "280" here?

The long snap name in format of "_${ENCRYPTED-SNAP-NAME}_${INO}", the 
max length will be 1 +256 + 16.


>
>
>>   	for (; i < rinfo->dir_nr; i++) {
>>   		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>> -		struct ceph_fname fname = { .dir	= inode,
>> -					    .name	= rde->name,
>> -					    .name_len	= rde->name_len,
>> -					    .ctext	= rde->altname,
>> -					    .ctext_len	= rde->altname_len };
>> -		u32 olen = oname.len;
>> -
>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>> -		if (err) {
>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>> -			       rde->name_len, rde->name, err);
>> -			goto out;
>> -		}
> I may be missing something, but if you rip this out, where does the
> decryption happen?

In ceph_readdir_prepopulate(), and it will set the rde->dentry, and here 
we can get the name from the dentry directly instead of decrypt it again.

>
>> +		struct dentry *dn = rde->dentry;
>> +		int name_len;
>>   
>>   		BUG_ON(rde->offset < ctx->pos);
>>   		BUG_ON(!rde->inode.in);
>> +		BUG_ON(!rde->dentry);
>>   
>>   		ctx->pos = rde->offset;
>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>> -		     i, rinfo->dir_nr, ctx->pos,
>> -		     rde->name_len, rde->name, &rde->inode.in);
>>   
>> -		if (!dir_emit(ctx, oname.name, oname.len,
>> +		spin_lock(&dn->d_lock);
>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>> +		name_len = dn->d_name.len;
>> +		spin_unlock(&dn->d_lock);
>> +
>> +		dentry_name[name_len] = '\0';
>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>> +
>> +		dput(dn);
>> +		rde->dentry = NULL;
>> +
>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>   			dout("filldir stopping us...\n");
>>   			err = 0;
>> +			for (; i < rinfo->dir_nr; i++) {
>> +				rde = rinfo->dir_entries + i;
>> +				dput(rde->dentry);
>> +				rde->dentry = NULL;
>> +			}
>>   			goto out;
>>   		}
>>   
>> -		/* Reset the lengths to their original allocated vals */
>> -		oname.len = olen;
>>   		ctx->pos++;
>>   	}
>>   
>> @@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>   	err = 0;
>>   	dout("readdir %p file %p done.\n", inode, file);
>>   out:
>> -	ceph_fname_free_buffer(inode, &tname);
>> -	ceph_fname_free_buffer(inode, &oname);
>> +	if (dentry_name)
>> +		kfree(dentry_name);
>>   	return err;
>>   }
>>   
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 2bc2f02b84e8..877e699fe43b 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>   			goto out;
>>   		}
>>   
>> +		rde->dentry = NULL;
>>   		dname.name = oname.name;
>>   		dname.len = oname.len;
>>   		dname.hash = full_name_hash(parent, dname.name, dname.len);
>> @@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>   			goto retry_lookup;
>>   		}
>>   
>> +		/*
>> +		 * ceph_readdir will use the dentry to get the name
>> +		 * to avoid doing the dencrypt again there.
>> +		 */
>> +		rde->dentry = dget(dn);
>> +
>>   		/* inode */
>>   		if (d_really_is_positive(dn)) {
>>   			in = d_inode(dn);
>> @@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>   		dput(dn);
>>   	}
>>   out:
>> +	if (err) {
>> +		for (; i >= 0; i--) {
>> +			struct ceph_mds_reply_dir_entry *rde;
>> +
>> +			rde = rinfo->dir_entries + i;
>> +			dput(rde->dentry);
>> +		}
>> +	}
>>   	if (err == 0 && skipped == 0) {
>>   		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
>>   		req->r_readdir_cache_idx = cache_ctl.index;
>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>> index 0dfe24f94567..663d7754d57d 100644
>> --- a/fs/ceph/mds_client.h
>> +++ b/fs/ceph/mds_client.h
>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>   };
>>   
>>   struct ceph_mds_reply_dir_entry {
>> +	struct dentry		      *dentry;
>>   	char                          *name;
>>   	u8			      *altname;
>>   	u32                           name_len;
>
> NAK on this patch as it is...
>

Re: [PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[Jeff Layton]

On Tue, 2022-03-01 at 22:05 +0800, Xiubo Li wrote:
> On 3/1/22 9:31 PM, Jeff Layton wrote:
> > On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
> > > From: Xiubo Li <xiubli@redhat.com>
> > > 
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > ---
> > >   fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
> > >   fs/ceph/inode.c      | 15 ++++++++++
> > >   fs/ceph/mds_client.h |  1 +
> > >   3 files changed, 47 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > index 6be0c1f793c2..e3917b4426e8 100644
> > > --- a/fs/ceph/dir.c
> > > +++ b/fs/ceph/dir.c
> > > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   	int err;
> > >   	unsigned frag = -1;
> > >   	struct ceph_mds_reply_info_parsed *rinfo;
> > > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > > +	char *dentry_name = NULL;
> > >   
> > >   	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> > >   	if (dfi->file_info.flags & CEPH_F_ATEND)
> > > @@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   		ctx->pos = 2;
> > >   	}
> > >   
> > > -	err = fscrypt_prepare_readdir(inode);
> > > -	if (err)
> > > -		goto out;
> > > -
> > 
> > Why are you removing this? This is what ensures that the key is loaded
> > if we're going to need it.
> 
> Since the dentry names are already decrypted in 
> ceph_readdir_prepopulate(), and this patch is trying to avoid do the 
> same thing again here after that.
> 
> Because in ceph_readdir() this isn't needed any more. And in other 
> places such as build path it will do it when needed.
> 
> > 
> > >   	spin_lock(&ci->i_ceph_lock);
> > >   	/* request Fx cap. if have Fx, we don't need to release Fs cap
> > >   	 * for later create/unlink. */
> > > @@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   		spin_unlock(&ci->i_ceph_lock);
> > >   	}
> > >   
> > > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > > -	if (err < 0)
> > > -		goto out;
> > > -
> > > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > > -	if (err < 0)
> > > -		goto out;
> > > -
> > >   	/* proceed with a normal readdir */
> > >   more:
> > >   	/* do we have the correct frag content buffered? */
> > > @@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   			}
> > >   		}
> > >   	}
> > > +
> > > +	dentry_name = kmalloc(280, GFP_KERNEL);
> > > +	if (!dentry_name) {
> > > +		err = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > Woah, what's up with the bare "280" here?
> 
> The long snap name in format of "_${ENCRYPTED-SNAP-NAME}_${INO}", the 
> max length will be 1 +256 + 16.
> 

Ok. Best to make a named constant definition for this then. Bare numbers
in an allocation like this make it hard to understand what's going on.

> 
> > 
> > 
> > >   	for (; i < rinfo->dir_nr; i++) {
> > >   		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > > -		struct ceph_fname fname = { .dir	= inode,
> > > -					    .name	= rde->name,
> > > -					    .name_len	= rde->name_len,
> > > -					    .ctext	= rde->altname,
> > > -					    .ctext_len	= rde->altname_len };
> > > -		u32 olen = oname.len;
> > > -
> > > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > -		if (err) {
> > > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > -			       rde->name_len, rde->name, err);
> > > -			goto out;
> > > -		}
> > I may be missing something, but if you rip this out, where does the
> > decryption happen?
> 
> In ceph_readdir_prepopulate(), and it will set the rde->dentry, and here 
> we can get the name from the dentry directly instead of decrypt it again.
> 
> > 
> > > +		struct dentry *dn = rde->dentry;
> > > +		int name_len;
> > >   
> > >   		BUG_ON(rde->offset < ctx->pos);
> > >   		BUG_ON(!rde->inode.in);
> > > +		BUG_ON(!rde->dentry);
> > >   
> > >   		ctx->pos = rde->offset;
> > > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > > -		     i, rinfo->dir_nr, ctx->pos,
> > > -		     rde->name_len, rde->name, &rde->inode.in);
> > >   
> > > -		if (!dir_emit(ctx, oname.name, oname.len,
> > > +		spin_lock(&dn->d_lock);
> > > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > > +		name_len = dn->d_name.len;
> > > +		spin_unlock(&dn->d_lock);
> > > +
> > > +		dentry_name[name_len] = '\0';
> > > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > > +
> > > +		dput(dn);
> > > +		rde->dentry = NULL;
> > > +
> > > +		if (!dir_emit(ctx, dentry_name, name_len,
> > >   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> > >   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> > >   			dout("filldir stopping us...\n");
> > >   			err = 0;
> > > +			for (; i < rinfo->dir_nr; i++) {
> > > +				rde = rinfo->dir_entries + i;
> > > +				dput(rde->dentry);
> > > +				rde->dentry = NULL;
> > > +			}
> > >   			goto out;
> > >   		}
> > >   
> > > -		/* Reset the lengths to their original allocated vals */
> > > -		oname.len = olen;
> > >   		ctx->pos++;
> > >   	}
> > >   
> > > @@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   	err = 0;
> > >   	dout("readdir %p file %p done.\n", inode, file);
> > >   out:
> > > -	ceph_fname_free_buffer(inode, &tname);
> > > -	ceph_fname_free_buffer(inode, &oname);
> > > +	if (dentry_name)
> > > +		kfree(dentry_name);
> > >   	return err;
> > >   }
> > >   
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 2bc2f02b84e8..877e699fe43b 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > >   			goto out;
> > >   		}
> > >   
> > > +		rde->dentry = NULL;
> > >   		dname.name = oname.name;
> > >   		dname.len = oname.len;
> > >   		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > > @@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > >   			goto retry_lookup;
> > >   		}
> > >   
> > > +		/*
> > > +		 * ceph_readdir will use the dentry to get the name
> > > +		 * to avoid doing the dencrypt again there.
> > > +		 */
> > > +		rde->dentry = dget(dn);
> > > +
> > >   		/* inode */
> > >   		if (d_really_is_positive(dn)) {
> > >   			in = d_inode(dn);
> > > @@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > >   		dput(dn);
> > >   	}
> > >   out:
> > > +	if (err) {
> > > +		for (; i >= 0; i--) {
> > > +			struct ceph_mds_reply_dir_entry *rde;
> > > +
> > > +			rde = rinfo->dir_entries + i;
> > > +			dput(rde->dentry);
> > > +		}
> > > +	}
> > >   	if (err == 0 && skipped == 0) {
> > >   		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
> > >   		req->r_readdir_cache_idx = cache_ctl.index;
> > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > index 0dfe24f94567..663d7754d57d 100644
> > > --- a/fs/ceph/mds_client.h
> > > +++ b/fs/ceph/mds_client.h
> > > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> > >   };
> > >   
> > >   struct ceph_mds_reply_dir_entry {
> > > +	struct dentry		      *dentry;
> > >   	char                          *name;
> > >   	u8			      *altname;
> > >   	u32                           name_len;
> > 
> > NAK on this patch as it is...
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[Xiubo Li]

On 3/1/22 10:18 PM, Jeff Layton wrote:
> On Tue, 2022-03-01 at 22:05 +0800, Xiubo Li wrote:
>> On 3/1/22 9:31 PM, Jeff Layton wrote:
>>> On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>
>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>> ---
>>>>    fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
>>>>    fs/ceph/inode.c      | 15 ++++++++++
>>>>    fs/ceph/mds_client.h |  1 +
>>>>    3 files changed, 47 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>> index 6be0c1f793c2..e3917b4426e8 100644
>>>> --- a/fs/ceph/dir.c
>>>> +++ b/fs/ceph/dir.c
>>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>    	int err;
>>>>    	unsigned frag = -1;
>>>>    	struct ceph_mds_reply_info_parsed *rinfo;
>>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>>> +	char *dentry_name = NULL;
>>>>    
>>>>    	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>>    	if (dfi->file_info.flags & CEPH_F_ATEND)
>>>> @@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>    		ctx->pos = 2;
>>>>    	}
>>>>    
>>>> -	err = fscrypt_prepare_readdir(inode);
>>>> -	if (err)
>>>> -		goto out;
>>>> -
>>> Why are you removing this? This is what ensures that the key is loaded
>>> if we're going to need it.
>> Since the dentry names are already decrypted in
>> ceph_readdir_prepopulate(), and this patch is trying to avoid do the
>> same thing again here after that.
>>
>> Because in ceph_readdir() this isn't needed any more. And in other
>> places such as build path it will do it when needed.
>>
>>>>    	spin_lock(&ci->i_ceph_lock);
>>>>    	/* request Fx cap. if have Fx, we don't need to release Fs cap
>>>>    	 * for later create/unlink. */
>>>> @@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>    		spin_unlock(&ci->i_ceph_lock);
>>>>    	}
>>>>    
>>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>>> -	if (err < 0)
>>>> -		goto out;
>>>> -
>>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>>> -	if (err < 0)
>>>> -		goto out;
>>>> -
>>>>    	/* proceed with a normal readdir */
>>>>    more:
>>>>    	/* do we have the correct frag content buffered? */
>>>> @@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>    			}
>>>>    		}
>>>>    	}
>>>> +
>>>> +	dentry_name = kmalloc(280, GFP_KERNEL);
>>>> +	if (!dentry_name) {
>>>> +		err = -ENOMEM;
>>>> +		goto out;
>>>> +	}
>>>> +
>>> Woah, what's up with the bare "280" here?
>> The long snap name in format of "_${ENCRYPTED-SNAP-NAME}_${INO}", the
>> max length will be 1 +256 + 16.
>>
> Ok. Best to make a named constant definition for this then. Bare numbers
> in an allocation like this make it hard to understand what's going on.
>
Sure.

So will this patch still makes sense ?

 From my test with thousands of dentries this could save a lot of time.


>>>
>>>>    	for (; i < rinfo->dir_nr; i++) {
>>>>    		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>>> -		struct ceph_fname fname = { .dir	= inode,
>>>> -					    .name	= rde->name,
>>>> -					    .name_len	= rde->name_len,
>>>> -					    .ctext	= rde->altname,
>>>> -					    .ctext_len	= rde->altname_len };
>>>> -		u32 olen = oname.len;
>>>> -
>>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>>> -		if (err) {
>>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>>> -			       rde->name_len, rde->name, err);
>>>> -			goto out;
>>>> -		}
>>> I may be missing something, but if you rip this out, where does the
>>> decryption happen?
>> In ceph_readdir_prepopulate(), and it will set the rde->dentry, and here
>> we can get the name from the dentry directly instead of decrypt it again.
>>
>>>> +		struct dentry *dn = rde->dentry;
>>>> +		int name_len;
>>>>    
>>>>    		BUG_ON(rde->offset < ctx->pos);
>>>>    		BUG_ON(!rde->inode.in);
>>>> +		BUG_ON(!rde->dentry);
>>>>    
>>>>    		ctx->pos = rde->offset;
>>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>>> -		     i, rinfo->dir_nr, ctx->pos,
>>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>>    
>>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>>> +		spin_lock(&dn->d_lock);
>>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>>> +		name_len = dn->d_name.len;
>>>> +		spin_unlock(&dn->d_lock);
>>>> +
>>>> +		dentry_name[name_len] = '\0';
>>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>>> +
>>>> +		dput(dn);
>>>> +		rde->dentry = NULL;
>>>> +
>>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>>    			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>>    			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>>    			dout("filldir stopping us...\n");
>>>>    			err = 0;
>>>> +			for (; i < rinfo->dir_nr; i++) {
>>>> +				rde = rinfo->dir_entries + i;
>>>> +				dput(rde->dentry);
>>>> +				rde->dentry = NULL;
>>>> +			}
>>>>    			goto out;
>>>>    		}
>>>>    
>>>> -		/* Reset the lengths to their original allocated vals */
>>>> -		oname.len = olen;
>>>>    		ctx->pos++;
>>>>    	}
>>>>    
>>>> @@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>    	err = 0;
>>>>    	dout("readdir %p file %p done.\n", inode, file);
>>>>    out:
>>>> -	ceph_fname_free_buffer(inode, &tname);
>>>> -	ceph_fname_free_buffer(inode, &oname);
>>>> +	if (dentry_name)
>>>> +		kfree(dentry_name);
>>>>    	return err;
>>>>    }
>>>>    
>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>> index 2bc2f02b84e8..877e699fe43b 100644
>>>> --- a/fs/ceph/inode.c
>>>> +++ b/fs/ceph/inode.c
>>>> @@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>    			goto out;
>>>>    		}
>>>>    
>>>> +		rde->dentry = NULL;
>>>>    		dname.name = oname.name;
>>>>    		dname.len = oname.len;
>>>>    		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>>> @@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>    			goto retry_lookup;
>>>>    		}
>>>>    
>>>> +		/*
>>>> +		 * ceph_readdir will use the dentry to get the name
>>>> +		 * to avoid doing the dencrypt again there.
>>>> +		 */
>>>> +		rde->dentry = dget(dn);
>>>> +
>>>>    		/* inode */
>>>>    		if (d_really_is_positive(dn)) {
>>>>    			in = d_inode(dn);
>>>> @@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>    		dput(dn);
>>>>    	}
>>>>    out:
>>>> +	if (err) {
>>>> +		for (; i >= 0; i--) {
>>>> +			struct ceph_mds_reply_dir_entry *rde;
>>>> +
>>>> +			rde = rinfo->dir_entries + i;
>>>> +			dput(rde->dentry);
>>>> +		}
>>>> +	}
>>>>    	if (err == 0 && skipped == 0) {
>>>>    		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
>>>>    		req->r_readdir_cache_idx = cache_ctl.index;
>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>> index 0dfe24f94567..663d7754d57d 100644
>>>> --- a/fs/ceph/mds_client.h
>>>> +++ b/fs/ceph/mds_client.h
>>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>>    };
>>>>    
>>>>    struct ceph_mds_reply_dir_entry {
>>>> +	struct dentry		      *dentry;
>>>>    	char                          *name;
>>>>    	u8			      *altname;
>>>>    	u32                           name_len;
>>> NAK on this patch as it is...
>>>

Re: [PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[Jeff Layton]

On Tue, 2022-03-01 at 22:33 +0800, Xiubo Li wrote:
> On 3/1/22 10:18 PM, Jeff Layton wrote:
> > On Tue, 2022-03-01 at 22:05 +0800, Xiubo Li wrote:
> > > On 3/1/22 9:31 PM, Jeff Layton wrote:
> > > > On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
> > > > > From: Xiubo Li <xiubli@redhat.com>
> > > > > 
> > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > > > ---
> > > > >    fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
> > > > >    fs/ceph/inode.c      | 15 ++++++++++
> > > > >    fs/ceph/mds_client.h |  1 +
> > > > >    3 files changed, 47 insertions(+), 35 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > > index 6be0c1f793c2..e3917b4426e8 100644
> > > > > --- a/fs/ceph/dir.c
> > > > > +++ b/fs/ceph/dir.c
> > > > > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > >    	int err;
> > > > >    	unsigned frag = -1;
> > > > >    	struct ceph_mds_reply_info_parsed *rinfo;
> > > > > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > > > > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > > > > +	char *dentry_name = NULL;
> > > > >    
> > > > >    	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> > > > >    	if (dfi->file_info.flags & CEPH_F_ATEND)
> > > > > @@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > >    		ctx->pos = 2;
> > > > >    	}
> > > > >    
> > > > > -	err = fscrypt_prepare_readdir(inode);
> > > > > -	if (err)
> > > > > -		goto out;
> > > > > -
> > > > Why are you removing this? This is what ensures that the key is loaded
> > > > if we're going to need it.
> > > Since the dentry names are already decrypted in
> > > ceph_readdir_prepopulate(), and this patch is trying to avoid do the
> > > same thing again here after that.
> > > 
> > > Because in ceph_readdir() this isn't needed any more. And in other
> > > places such as build path it will do it when needed.
> > > 
> > > > >    	spin_lock(&ci->i_ceph_lock);
> > > > >    	/* request Fx cap. if have Fx, we don't need to release Fs cap
> > > > >    	 * for later create/unlink. */
> > > > > @@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > >    		spin_unlock(&ci->i_ceph_lock);
> > > > >    	}
> > > > >    
> > > > > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > > > > -	if (err < 0)
> > > > > -		goto out;
> > > > > -
> > > > > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > > > > -	if (err < 0)
> > > > > -		goto out;
> > > > > -
> > > > >    	/* proceed with a normal readdir */
> > > > >    more:
> > > > >    	/* do we have the correct frag content buffered? */
> > > > > @@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > >    			}
> > > > >    		}
> > > > >    	}
> > > > > +
> > > > > +	dentry_name = kmalloc(280, GFP_KERNEL);
> > > > > +	if (!dentry_name) {
> > > > > +		err = -ENOMEM;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > Woah, what's up with the bare "280" here?
> > > The long snap name in format of "_${ENCRYPTED-SNAP-NAME}_${INO}", the
> > > max length will be 1 +256 + 16.
> > > 
> > Ok. Best to make a named constant definition for this then. Bare numbers
> > in an allocation like this make it hard to understand what's going on.
> > 
> Sure.
> 
> So will this patch still makes sense ?
> 
>  From my test with thousands of dentries this could save a lot of time.
> 
> 

Yeah, I think so.

It probably would have made more sense if you had described in the
commit message how we end up decrypting the names twice, and how this
patch addresses that.

> > > > 
> > > > >    	for (; i < rinfo->dir_nr; i++) {
> > > > >    		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > > > > -		struct ceph_fname fname = { .dir	= inode,
> > > > > -					    .name	= rde->name,
> > > > > -					    .name_len	= rde->name_len,
> > > > > -					    .ctext	= rde->altname,
> > > > > -					    .ctext_len	= rde->altname_len };
> > > > > -		u32 olen = oname.len;
> > > > > -
> > > > > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > > > -		if (err) {
> > > > > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > > > -			       rde->name_len, rde->name, err);
> > > > > -			goto out;
> > > > > -		}
> > > > I may be missing something, but if you rip this out, where does the
> > > > decryption happen?
> > > In ceph_readdir_prepopulate(), and it will set the rde->dentry, and here
> > > we can get the name from the dentry directly instead of decrypt it again.
> > > 
> > > > > +		struct dentry *dn = rde->dentry;
> > > > > +		int name_len;
> > > > >    
> > > > >    		BUG_ON(rde->offset < ctx->pos);
> > > > >    		BUG_ON(!rde->inode.in);
> > > > > +		BUG_ON(!rde->dentry);
> > > > >    
> > > > >    		ctx->pos = rde->offset;
> > > > > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > > > > -		     i, rinfo->dir_nr, ctx->pos,
> > > > > -		     rde->name_len, rde->name, &rde->inode.in);
> > > > >    
> > > > > -		if (!dir_emit(ctx, oname.name, oname.len,
> > > > > +		spin_lock(&dn->d_lock);
> > > > > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > > > > +		name_len = dn->d_name.len;
> > > > > +		spin_unlock(&dn->d_lock);
> > > > > +
> > > > > +		dentry_name[name_len] = '\0';
> > > > > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > > > > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > > > > +
> > > > > +		dput(dn);
> > > > > +		rde->dentry = NULL;
> > > > > +
> > > > > +		if (!dir_emit(ctx, dentry_name, name_len,
> > > > >    			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> > > > >    			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> > > > >    			dout("filldir stopping us...\n");
> > > > >    			err = 0;
> > > > > +			for (; i < rinfo->dir_nr; i++) {
> > > > > +				rde = rinfo->dir_entries + i;
> > > > > +				dput(rde->dentry);
> > > > > +				rde->dentry = NULL;
> > > > > +			}
> > > > >    			goto out;
> > > > >    		}
> > > > >    
> > > > > -		/* Reset the lengths to their original allocated vals */
> > > > > -		oname.len = olen;
> > > > >    		ctx->pos++;
> > > > >    	}
> > > > >    
> > > > > @@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > >    	err = 0;
> > > > >    	dout("readdir %p file %p done.\n", inode, file);
> > > > >    out:
> > > > > -	ceph_fname_free_buffer(inode, &tname);
> > > > > -	ceph_fname_free_buffer(inode, &oname);
> > > > > +	if (dentry_name)
> > > > > +		kfree(dentry_name);
> > > > >    	return err;
> > > > >    }
> > > > >    
> > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > index 2bc2f02b84e8..877e699fe43b 100644
> > > > > --- a/fs/ceph/inode.c
> > > > > +++ b/fs/ceph/inode.c
> > > > > @@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > >    			goto out;
> > > > >    		}
> > > > >    
> > > > > +		rde->dentry = NULL;
> > > > >    		dname.name = oname.name;
> > > > >    		dname.len = oname.len;
> > > > >    		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > > > > @@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > >    			goto retry_lookup;
> > > > >    		}
> > > > >    
> > > > > +		/*
> > > > > +		 * ceph_readdir will use the dentry to get the name
> > > > > +		 * to avoid doing the dencrypt again there.
> > > > > +		 */
> > > > > +		rde->dentry = dget(dn);
> > > > > +
> > > > >    		/* inode */
> > > > >    		if (d_really_is_positive(dn)) {
> > > > >    			in = d_inode(dn);
> > > > > @@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > >    		dput(dn);
> > > > >    	}
> > > > >    out:
> > > > > +	if (err) {
> > > > > +		for (; i >= 0; i--) {
> > > > > +			struct ceph_mds_reply_dir_entry *rde;
> > > > > +
> > > > > +			rde = rinfo->dir_entries + i;
> > > > > +			dput(rde->dentry);
> > > > > +		}
> > > > > +	}
> > > > >    	if (err == 0 && skipped == 0) {
> > > > >    		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
> > > > >    		req->r_readdir_cache_idx = cache_ctl.index;
> > > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > > index 0dfe24f94567..663d7754d57d 100644
> > > > > --- a/fs/ceph/mds_client.h
> > > > > +++ b/fs/ceph/mds_client.h
> > > > > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> > > > >    };
> > > > >    
> > > > >    struct ceph_mds_reply_dir_entry {
> > > > > +	struct dentry		      *dentry;
> > > > >    	char                          *name;
> > > > >    	u8			      *altname;
> > > > >    	u32                           name_len;
> > > > NAK on this patch as it is...
> > > > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 3/7] ceph: do not dencrypt the dentry name twice for readdir

[Xiubo Li]

On 3/1/22 10:43 PM, Jeff Layton wrote:
> On Tue, 2022-03-01 at 22:33 +0800, Xiubo Li wrote:
>> On 3/1/22 10:18 PM, Jeff Layton wrote:
>>> On Tue, 2022-03-01 at 22:05 +0800, Xiubo Li wrote:
>>>> On 3/1/22 9:31 PM, Jeff Layton wrote:
>>>>> On Tue, 2022-03-01 at 19:30 +0800, xiubli@redhat.com wrote:
>>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>>
>>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>>> ---
>>>>>>     fs/ceph/dir.c        | 66 +++++++++++++++++++++-----------------------
>>>>>>     fs/ceph/inode.c      | 15 ++++++++++
>>>>>>     fs/ceph/mds_client.h |  1 +
>>>>>>     3 files changed, 47 insertions(+), 35 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>>> index 6be0c1f793c2..e3917b4426e8 100644
>>>>>> --- a/fs/ceph/dir.c
>>>>>> +++ b/fs/ceph/dir.c
>>>>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>     	int err;
>>>>>>     	unsigned frag = -1;
>>>>>>     	struct ceph_mds_reply_info_parsed *rinfo;
>>>>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>>>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>>>>> +	char *dentry_name = NULL;
>>>>>>     
>>>>>>     	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>>>>     	if (dfi->file_info.flags & CEPH_F_ATEND)
>>>>>> @@ -345,10 +344,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>     		ctx->pos = 2;
>>>>>>     	}
>>>>>>     
>>>>>> -	err = fscrypt_prepare_readdir(inode);
>>>>>> -	if (err)
>>>>>> -		goto out;
>>>>>> -
>>>>> Why are you removing this? This is what ensures that the key is loaded
>>>>> if we're going to need it.
>>>> Since the dentry names are already decrypted in
>>>> ceph_readdir_prepopulate(), and this patch is trying to avoid do the
>>>> same thing again here after that.
>>>>
>>>> Because in ceph_readdir() this isn't needed any more. And in other
>>>> places such as build path it will do it when needed.
>>>>
>>>>>>     	spin_lock(&ci->i_ceph_lock);
>>>>>>     	/* request Fx cap. if have Fx, we don't need to release Fs cap
>>>>>>     	 * for later create/unlink. */
>>>>>> @@ -369,14 +364,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>     		spin_unlock(&ci->i_ceph_lock);
>>>>>>     	}
>>>>>>     
>>>>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>>>>> -	if (err < 0)
>>>>>> -		goto out;
>>>>>> -
>>>>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>>>>> -	if (err < 0)
>>>>>> -		goto out;
>>>>>> -
>>>>>>     	/* proceed with a normal readdir */
>>>>>>     more:
>>>>>>     	/* do we have the correct frag content buffered? */
>>>>>> @@ -525,40 +512,49 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>     			}
>>>>>>     		}
>>>>>>     	}
>>>>>> +
>>>>>> +	dentry_name = kmalloc(280, GFP_KERNEL);
>>>>>> +	if (!dentry_name) {
>>>>>> +		err = -ENOMEM;
>>>>>> +		goto out;
>>>>>> +	}
>>>>>> +
>>>>> Woah, what's up with the bare "280" here?
>>>> The long snap name in format of "_${ENCRYPTED-SNAP-NAME}_${INO}", the
>>>> max length will be 1 +256 + 16.
>>>>
>>> Ok. Best to make a named constant definition for this then. Bare numbers
>>> in an allocation like this make it hard to understand what's going on.
>>>
>> Sure.
>>
>> So will this patch still makes sense ?
>>
>>   From my test with thousands of dentries this could save a lot of time.
>>
>>
> Yeah, I think so.
>
> It probably would have made more sense if you had described in the
> commit message how we end up decrypting the names twice, and how this
> patch addresses that.

Sure, will do that.


>
>>>>>>     	for (; i < rinfo->dir_nr; i++) {
>>>>>>     		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>>>>> -		struct ceph_fname fname = { .dir	= inode,
>>>>>> -					    .name	= rde->name,
>>>>>> -					    .name_len	= rde->name_len,
>>>>>> -					    .ctext	= rde->altname,
>>>>>> -					    .ctext_len	= rde->altname_len };
>>>>>> -		u32 olen = oname.len;
>>>>>> -
>>>>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>>>>> -		if (err) {
>>>>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>>>>> -			       rde->name_len, rde->name, err);
>>>>>> -			goto out;
>>>>>> -		}
>>>>> I may be missing something, but if you rip this out, where does the
>>>>> decryption happen?
>>>> In ceph_readdir_prepopulate(), and it will set the rde->dentry, and here
>>>> we can get the name from the dentry directly instead of decrypt it again.
>>>>
>>>>>> +		struct dentry *dn = rde->dentry;
>>>>>> +		int name_len;
>>>>>>     
>>>>>>     		BUG_ON(rde->offset < ctx->pos);
>>>>>>     		BUG_ON(!rde->inode.in);
>>>>>> +		BUG_ON(!rde->dentry);
>>>>>>     
>>>>>>     		ctx->pos = rde->offset;
>>>>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>>>>> -		     i, rinfo->dir_nr, ctx->pos,
>>>>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>>>>     
>>>>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>>>>> +		spin_lock(&dn->d_lock);
>>>>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>>>>> +		name_len = dn->d_name.len;
>>>>>> +		spin_unlock(&dn->d_lock);
>>>>>> +
>>>>>> +		dentry_name[name_len] = '\0';
>>>>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>>>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>>>>> +
>>>>>> +		dput(dn);
>>>>>> +		rde->dentry = NULL;
>>>>>> +
>>>>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>>>>     			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>>>>     			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>>>>     			dout("filldir stopping us...\n");
>>>>>>     			err = 0;
>>>>>> +			for (; i < rinfo->dir_nr; i++) {
>>>>>> +				rde = rinfo->dir_entries + i;
>>>>>> +				dput(rde->dentry);
>>>>>> +				rde->dentry = NULL;
>>>>>> +			}
>>>>>>     			goto out;
>>>>>>     		}
>>>>>>     
>>>>>> -		/* Reset the lengths to their original allocated vals */
>>>>>> -		oname.len = olen;
>>>>>>     		ctx->pos++;
>>>>>>     	}
>>>>>>     
>>>>>> @@ -616,8 +612,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>     	err = 0;
>>>>>>     	dout("readdir %p file %p done.\n", inode, file);
>>>>>>     out:
>>>>>> -	ceph_fname_free_buffer(inode, &tname);
>>>>>> -	ceph_fname_free_buffer(inode, &oname);
>>>>>> +	if (dentry_name)
>>>>>> +		kfree(dentry_name);
>>>>>>     	return err;
>>>>>>     }
>>>>>>     
>>>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>>>> index 2bc2f02b84e8..877e699fe43b 100644
>>>>>> --- a/fs/ceph/inode.c
>>>>>> +++ b/fs/ceph/inode.c
>>>>>> @@ -1903,6 +1903,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>     			goto out;
>>>>>>     		}
>>>>>>     
>>>>>> +		rde->dentry = NULL;
>>>>>>     		dname.name = oname.name;
>>>>>>     		dname.len = oname.len;
>>>>>>     		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>>>>> @@ -1963,6 +1964,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>     			goto retry_lookup;
>>>>>>     		}
>>>>>>     
>>>>>> +		/*
>>>>>> +		 * ceph_readdir will use the dentry to get the name
>>>>>> +		 * to avoid doing the dencrypt again there.
>>>>>> +		 */
>>>>>> +		rde->dentry = dget(dn);
>>>>>> +
>>>>>>     		/* inode */
>>>>>>     		if (d_really_is_positive(dn)) {
>>>>>>     			in = d_inode(dn);
>>>>>> @@ -2025,6 +2032,14 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>     		dput(dn);
>>>>>>     	}
>>>>>>     out:
>>>>>> +	if (err) {
>>>>>> +		for (; i >= 0; i--) {
>>>>>> +			struct ceph_mds_reply_dir_entry *rde;
>>>>>> +
>>>>>> +			rde = rinfo->dir_entries + i;
>>>>>> +			dput(rde->dentry);
>>>>>> +		}
>>>>>> +	}
>>>>>>     	if (err == 0 && skipped == 0) {
>>>>>>     		set_bit(CEPH_MDS_R_DID_PREPOPULATE, &req->r_req_flags);
>>>>>>     		req->r_readdir_cache_idx = cache_ctl.index;
>>>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>>>> index 0dfe24f94567..663d7754d57d 100644
>>>>>> --- a/fs/ceph/mds_client.h
>>>>>> +++ b/fs/ceph/mds_client.h
>>>>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>>>>     };
>>>>>>     
>>>>>>     struct ceph_mds_reply_dir_entry {
>>>>>> +	struct dentry		      *dentry;
>>>>>>     	char                          *name;
>>>>>>     	u8			      *altname;
>>>>>>     	u32                           name_len;
>>>>> NAK on this patch as it is...
>>>>>

[PATCH v2 4/7] ceph: add ceph_get_snap_parent_inode() support

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

Get the parent inode for the snap directory ".snap", if the inode
is not a snap directory just return it with the reference increased.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/snap.c  | 24 ++++++++++++++++++++++++
 fs/ceph/super.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index 66a1a92cf579..5e9656926dc7 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -1247,3 +1247,27 @@ void ceph_cleanup_snapid_map(struct ceph_mds_client *mdsc)
 		kfree(sm);
 	}
 }
+
+/*
+ * Get the parent inode for the snap directory ".snap",
+ * if the inode is not a snap directory just return it
+ * with the reference increased.
+ */
+struct inode *ceph_get_snap_parent_inode(struct inode *inode)
+{
+	struct inode *pinode;
+
+	if (ceph_snap(inode) == CEPH_SNAPDIR) {
+		struct ceph_vino vino = {
+			.ino = ceph_ino(inode),
+			.snap = CEPH_NOSNAP,
+		};
+		pinode = ceph_find_inode(inode->i_sb, vino);
+		BUG_ON(!pinode);
+	} else {
+		ihold(inode);
+		pinode = inode;
+	}
+
+	return pinode;
+}
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index d5a4d311f4c2..e7c69ca5e289 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -969,6 +969,7 @@ extern void ceph_put_snapid_map(struct ceph_mds_client* mdsc,
 				struct ceph_snapid_map *sm);
 extern void ceph_trim_snapid_map(struct ceph_mds_client *mdsc);
 extern void ceph_cleanup_snapid_map(struct ceph_mds_client *mdsc);
+extern struct inode *ceph_get_snap_parent_inode(struct inode *inode);
 void ceph_umount_begin(struct super_block *sb);
 
 
-- 
2.27.0

[PATCH v2 5/7] ceph: use the parent inode of '.snap' to dencrypt the names for readdir

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The inode for '.snap' directory will always with no key setup, so
we can use the parent inode to do this.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/inode.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 877e699fe43b..a6c4965e5363 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1817,7 +1817,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info;
 	struct qstr dname;
 	struct dentry *dn;
-	struct inode *in;
+	struct inode *in, *pinode;
 	int err = 0, skipped = 0, ret, i;
 	u32 frag = le32_to_cpu(req->r_args.readdir.frag);
 	u32 last_hash = 0;
@@ -1876,11 +1876,13 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	cache_ctl.index = req->r_readdir_cache_idx;
 	fpos_offset = req->r_readdir_offset;
 
-	err = ceph_fname_alloc_buffer(inode, &tname);
+	pinode = ceph_get_snap_parent_inode(inode);
+
+	err = ceph_fname_alloc_buffer(pinode, &tname);
 	if (err < 0)
 		goto out;
 
-	err = ceph_fname_alloc_buffer(inode, &oname);
+	err = ceph_fname_alloc_buffer(pinode, &oname);
 	if (err < 0)
 		goto out;
 
@@ -1890,7 +1892,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
 		struct ceph_vino tvino;
 		u32 olen = oname.len;
-		struct ceph_fname fname = { .dir	= inode,
+		struct ceph_fname fname = { .dir	= pinode,
 					    .name	= rde->name,
 					    .name_len	= rde->name_len,
 					    .ctext	= rde->altname,
@@ -2045,8 +2047,9 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		req->r_readdir_cache_idx = cache_ctl.index;
 	}
 	ceph_readdir_cache_release(&cache_ctl);
-	ceph_fname_free_buffer(inode, &tname);
-	ceph_fname_free_buffer(inode, &oname);
+	ceph_fname_free_buffer(pinode, &tname);
+	ceph_fname_free_buffer(pinode, &oname);
+	iput(pinode);
 	dout("readdir_prepopulate done\n");
 	return err;
 }
-- 
2.27.0

[PATCH v2 6/7] ceph: use the parent inode of '.snap' to encrypt name to build path

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The inode for '.snap' directory will always with no key setup, so
we can use the parent inode to do this.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/mds_client.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 3dea96df4769..5433e6050563 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2465,8 +2465,8 @@ static u8 *get_fscrypt_altname(const struct ceph_mds_request *req, u32 *plen)
  */
 char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire)
 {
-	struct dentry *cur;
-	struct inode *inode;
+	struct dentry *cur, *parent;
+	struct inode *inode, *pinode;
 	char *path;
 	int pos;
 	unsigned seq;
@@ -2479,13 +2479,16 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for
 	if (!path)
 		return ERR_PTR(-ENOMEM);
 retry:
+	pinode = NULL;
+	parent = NULL;
 	pos = PATH_MAX - 1;
 	path[pos] = '\0';
 
 	seq = read_seqbegin(&rename_lock);
 	cur = dget(dentry);
 	for (;;) {
-		struct dentry *parent;
+		parent = dget_parent(cur);
+		pinode = ceph_get_snap_parent_inode(d_inode(parent));
 
 		spin_lock(&cur->d_lock);
 		inode = d_inode(cur);
@@ -2493,12 +2496,11 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for
 			dout("build_path path+%d: %p SNAPDIR\n",
 			     pos, cur);
 			spin_unlock(&cur->d_lock);
-			parent = dget_parent(cur);
 		} 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 if (!for_wire || !IS_ENCRYPTED(d_inode(cur->d_parent))) {
+		} else if (!for_wire || !IS_ENCRYPTED(pinode)) {
 			pos -= cur->d_name.len;
 			if (pos < 0) {
 				spin_unlock(&cur->d_lock);
@@ -2506,7 +2508,6 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for
 			}
 			memcpy(path + pos, cur->d_name.name, cur->d_name.len);
 			spin_unlock(&cur->d_lock);
-			parent = dget_parent(cur);
 		} else {
 			int len, ret;
 			char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
@@ -2518,32 +2519,32 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for
 			memcpy(buf, cur->d_name.name, cur->d_name.len);
 			len = cur->d_name.len;
 			spin_unlock(&cur->d_lock);
-			parent = dget_parent(cur);
 
-			ret = __fscrypt_prepare_readdir(d_inode(parent));
+			ret = __fscrypt_prepare_readdir(pinode);
 			if (ret < 0) {
 				dput(parent);
 				dput(cur);
+				iput(pinode);
 				return ERR_PTR(ret);
 			}
 
-			if (fscrypt_has_encryption_key(d_inode(parent))) {
-				len = ceph_encode_encrypted_fname(d_inode(parent), cur, buf);
+			if (fscrypt_has_encryption_key(pinode)) {
+				len = ceph_encode_encrypted_fname(pinode, cur, buf);
 				if (len < 0) {
 					dput(parent);
 					dput(cur);
+					iput(pinode);
 					return ERR_PTR(len);
 				}
 			}
 			pos -= len;
-			if (pos < 0) {
-				dput(parent);
+			if (pos < 0)
 				break;
-			}
 			memcpy(path + pos, buf, len);
 		}
 		dput(cur);
 		cur = parent;
+		parent = NULL;
 
 		/* Are we at the root? */
 		if (IS_ROOT(cur))
@@ -2554,7 +2555,13 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for
 			break;
 
 		path[pos] = '/';
+		iput(pinode);
+		pinode = NULL;
 	}
+	if (pinode)
+		iput(pinode);
+	if (parent)
+		dput(parent);
 	inode = d_inode(cur);
 	base = inode ? ceph_ino(inode) : 0;
 	dput(cur);
-- 
2.27.0

[PATCH v2 7/7] ceph: try to encrypt/decrypt long snap name

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The child realm will inherit parents' snapshots, and the snapshot
names will be in long name format:

  "_${ENCRYPTED-NAME}_${PARENT-INO}"

We need to parse the ENCRYPTED-NAME and to the decryption for readdir
and when lookup a snapshot we also need to encrypt the real snapshot
name and then switch to the long snap name to do the lookup in MDS.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/crypto.c     | 75 ++++++++++++++++++++++++++++++++++++----
 fs/ceph/crypto.h     |  2 +-
 fs/ceph/dir.c        | 26 +++++++++++---
 fs/ceph/inode.c      | 81 ++++++++++++++++++++++++++++++++++++++++----
 fs/ceph/mds_client.c | 22 ++++++++----
 fs/ceph/mds_client.h |  2 ++
 fs/ceph/super.h      |  1 +
 7 files changed, 184 insertions(+), 25 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 5a87e7385d3f..3af0d7a0ed10 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -128,14 +128,61 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
 	swap(req->r_fscrypt_auth, as->fscrypt_auth);
 }
 
-int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf)
+int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
 {
+	struct ceph_dentry_info *di = ceph_dentry(dentry);
+	struct qstr d_name = {.len = 0, .name = NULL};
+	struct inode *pinode = parent;
 	u32 len;
 	int elen;
 	int ret;
 	u8 *cryptbuf;
+	char *p;
+	unsigned char *last = NULL;
 
-	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
+	// The long snap name format is "_${SNAP-NAME}_{INO}"
+	if (di->long_snap_name) {
+		struct ceph_vino vino = { .snap = CEPH_NOSNAP };
+
+		last = strrchr(dentry->d_name.name, '_');
+		if (!last)
+			return -EINVAL;
+
+		ret = kstrtou64(last + 1, 0, &vino.ino);
+		if (ret) {
+			kfree(d_name.name);
+			return ret;
+		}
+		pinode = ceph_get_inode(parent->i_sb, vino, NULL);
+		ret = __fscrypt_prepare_readdir(pinode);
+		if (ret < 0) {
+			iput(pinode);
+			return ret;
+		}
+		if (!fscrypt_has_encryption_key(pinode)) {
+			memcpy(buf, dentry->d_name.name, dentry->d_name.len);
+			buf[dentry->d_name.len] = '\0';
+			iput(pinode);
+			return dentry->d_name.len;
+		}
+
+		d_name.len = last - dentry->d_name.name - 1;
+		d_name.name = kstrndup(dentry->d_name.name + 1,
+				       d_name.len, GFP_KERNEL);
+		if (!d_name.name)
+			return -ENOMEM;
+		p = buf + 1;
+		buf[0] = '_';
+		dout(" long_snap_name real snap name: %s, ino: %s\n",
+		     d_name.name, last + 1);
+	} else {
+		p = buf;
+		d_name.name = dentry->d_name.name;
+		d_name.len = dentry->d_name.len;
+		ihold(parent);
+	}
+
+	WARN_ON_ONCE(!fscrypt_has_encryption_key(pinode));
 
 	/*
 	 * convert cleartext dentry name to ciphertext
@@ -144,20 +191,31 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
 	 *
 	 * See: fscrypt_setup_filename
 	 */
-	if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len))
+	if (!fscrypt_fname_encrypted_size(pinode, d_name.len, NAME_MAX, &len)) {
+		iput(pinode);
 		return -ENAMETOOLONG;
+	}
 
 	/* Allocate a buffer appropriate to hold the result */
 	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
-	if (!cryptbuf)
+	if (!cryptbuf) {
+		iput(pinode);
+		if (di->long_snap_name)
+			kfree(d_name.name);
 		return -ENOMEM;
+	}
 
-	ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len);
+	ret = fscrypt_fname_encrypt(pinode, &d_name, cryptbuf, len);
 	if (ret) {
+		iput(pinode);
 		kfree(cryptbuf);
+		if (di->long_snap_name)
+			kfree(d_name.name);
 		return ret;
 	}
 
+	iput(pinode);
+
 	/* hash the end if the name is long enough */
 	if (len > CEPH_NOHASH_NAME_MAX) {
 		u8 hash[SHA256_DIGEST_SIZE];
@@ -170,8 +228,13 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
 	}
 
 	/* base64 encode the encrypted name */
-	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
+	elen = fscrypt_base64url_encode(cryptbuf, len, p);
 	kfree(cryptbuf);
+	if (di->long_snap_name) {
+		kfree(d_name.name);
+		strcpy(p + elen, last);
+		elen += 1 + strlen(last);
+	}
 	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
 	return elen;
 }
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index 1e08f8a64ad6..e99b457a776a 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -90,7 +90,7 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
 int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
 				 struct ceph_acl_sec_ctx *as);
 void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
-int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
+int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
 {
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index e3917b4426e8..86c04e248f07 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -50,6 +50,7 @@ static int ceph_d_init(struct dentry *dentry)
 	di->time = jiffies;
 	dentry->d_fsdata = di;
 	INIT_LIST_HEAD(&di->lease_list);
+	di->long_snap_name = false;
 
 	atomic64_inc(&mdsc->metric.total_dentries);
 
@@ -547,7 +548,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
 			dout("filldir stopping us...\n");
 			err = 0;
-			for (; i < rinfo->dir_nr; i++) {
+			for (++i; i < rinfo->dir_nr; i++) {
 				rde = rinfo->dir_entries + i;
 				dput(rde->dentry);
 				rde->dentry = NULL;
@@ -775,7 +776,9 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 {
 	struct ceph_fs_client *fsc = ceph_sb_to_client(dir->i_sb);
 	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(dir->i_sb);
+	struct ceph_dentry_info *di = ceph_dentry(dentry);
 	struct ceph_mds_request *req;
+	struct inode *pinode = dir;
 	int op;
 	int mask;
 	int err;
@@ -786,21 +789,22 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 	if (dentry->d_name.len > NAME_MAX)
 		return ERR_PTR(-ENAMETOOLONG);
 
-	if (IS_ENCRYPTED(dir)) {
-		err = __fscrypt_prepare_readdir(dir);
+	pinode = ceph_get_snap_parent_inode(dir);
+	if (IS_ENCRYPTED(pinode)) {
+		err = __fscrypt_prepare_readdir(pinode);
 		if (err)
 			return ERR_PTR(err);
-		if (!fscrypt_has_encryption_key(dir)) {
+		if (!fscrypt_has_encryption_key(pinode)) {
 			spin_lock(&dentry->d_lock);
 			dentry->d_flags |= DCACHE_NOKEY_NAME;
 			spin_unlock(&dentry->d_lock);
 		}
 	}
+	iput(pinode);
 
 	/* can we conclude ENOENT locally? */
 	if (d_really_is_negative(dentry)) {
 		struct ceph_inode_info *ci = ceph_inode(dir);
-		struct ceph_dentry_info *di = ceph_dentry(dentry);
 
 		spin_lock(&ci->i_ceph_lock);
 		dout(" dir %p flags are 0x%lx\n", dir, ci->i_ceph_flags);
@@ -823,6 +827,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 
 	op = ceph_snap(dir) == CEPH_SNAPDIR ?
 		CEPH_MDS_OP_LOOKUPSNAP : CEPH_MDS_OP_LOOKUP;
+retry:
 	req = ceph_mdsc_create_request(mdsc, op, USE_ANY_MDS);
 	if (IS_ERR(req))
 		return ERR_CAST(req);
@@ -841,6 +846,16 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 	if (err == -ENOENT) {
 		struct dentry *res;
 
+		// long snap name with the format "_${SNAP-NAME}_${INO}" ?
+		if (IS_ENCRYPTED(pinode) && !di->long_snap_name &&
+		    op == CEPH_MDS_OP_LOOKUPSNAP &&
+		    dentry->d_name.name[0] == '_') {
+			di->long_snap_name = true;
+			ceph_mdsc_put_request(req);
+			dout("lookup retry with long snap name set.\n");
+			goto retry;
+		}
+
 		res = ceph_handle_snapdir(req, dentry);
 		if (IS_ERR(res)) {
 			err = PTR_ERR(res);
@@ -848,6 +863,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 			dentry = res;
 			err = 0;
 		}
+		di->long_snap_name = false;
 	}
 	dentry = ceph_finish_lookup(req, dentry, err);
 	ceph_mdsc_put_request(req);  /* will dput(dentry) */
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index a6c4965e5363..ec0c71b08b7c 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -1825,6 +1825,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	struct ceph_readdir_cache_control cache_ctl = {};
 	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
 	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
+	char *long_snap_name = NULL;
 
 	if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
 		return readdir_prepopulate_inodes_only(req, session);
@@ -1886,23 +1887,88 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	if (err < 0)
 		goto out;
 
+	long_snap_name = kzalloc(512, GFP_NOFS);
+	if (!long_snap_name) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	/* FIXME: release caps/leases if error occurs */
 	for (i = 0; i < rinfo->dir_nr; i++) {
 		bool is_nokey = false;
 		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
 		struct ceph_vino tvino;
 		u32 olen = oname.len;
+		struct ceph_dentry_info *di;
 		struct ceph_fname fname = { .dir	= pinode,
 					    .name	= rde->name,
 					    .name_len	= rde->name_len,
 					    .ctext	= rde->altname,
 					    .ctext_len	= rde->altname_len };
 
-		err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
-		if (err) {
-			pr_err("%s unable to decode %.*s, got %d\n", __func__,
-			       rde->name_len, rde->name, err);
-			goto out;
+		/* The long snap name will be "_${SNAP-NAME}_${INO}" */
+		if (rde->long_snap_name) {
+			int len;
+			char *lsn, *last, ino_str[20];
+			struct inode *_pinode;
+			struct ceph_vino vino = {
+				.snap = CEPH_NOSNAP,
+			};
+
+			// get the inode number of ${INO}
+			memcpy(long_snap_name, rde->name, rde->name_len);
+			long_snap_name[rde->name_len] = '\0';
+			last = strrchr(long_snap_name, '_');
+			if (!last) {
+				pr_err("%s long snapshot name %.*s badness\n",
+				       __func__, rde->name_len, rde->name);
+				goto out;
+			}
+			last++;
+			len = rde->name_len - (last - long_snap_name);
+			memcpy(ino_str, last, len);
+			ino_str[len] = '\0';
+			err = kstrtou64(ino_str, 0, &vino.ino);
+			if (err)
+				goto out;
+			_pinode = ceph_find_inode(inode->i_sb, vino);
+			BUG_ON(!_pinode);
+
+			// is the ${INO} encrypted ?
+			if (IS_ENCRYPTED(_pinode)) {
+				len = rde->name_len - 2 - len;
+				fname.dir = _pinode;
+				fname.name = rde->name + 1;
+				fname.name_len = len;
+
+				err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
+				if (err) {
+					pr_err("%s unable to decode %.*s, got %d\n", __func__,
+						rde->name_len, rde->name, err);
+					iput(_pinode);
+					goto out;
+				}
+				lsn = kasprintf(GFP_NOFS, "_%s_%s", oname.name, ino_str);
+				if (!lsn) {
+					err = -ENOMEM;
+					iput(_pinode);
+					goto out;
+				}
+				len = strlen(lsn);
+				memcpy(oname.name, lsn, len);
+				oname.len = len;
+			} else {
+				memcpy(oname.name, fname.name, fname.name_len);
+				oname.len = fname.name_len;
+			}
+			iput(_pinode);
+		} else {
+			err = ceph_fname_to_usr(&fname, &tname, &oname, &is_nokey);
+			if (err) {
+				pr_err("%s unable to decode %.*s, got %d\n", __func__,
+				       rde->name_len, rde->name, err);
+				goto out;
+			}
 		}
 
 		rde->dentry = NULL;
@@ -1948,7 +2014,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		} else if (d_really_is_positive(dn) &&
 			   (ceph_ino(d_inode(dn)) != tvino.ino ||
 			    ceph_snap(d_inode(dn)) != tvino.snap)) {
-			struct ceph_dentry_info *di = ceph_dentry(dn);
+			di = ceph_dentry(dn);
 			dout(" dn %p points to wrong inode %p\n",
 			     dn, d_inode(dn));
 
@@ -1971,6 +2037,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 		 * to avoid doing the dencrypt again there.
 		 */
 		rde->dentry = dget(dn);
+		di = ceph_dentry(dn);
+		di->long_snap_name = !!rde->long_snap_name;
 
 		/* inode */
 		if (d_really_is_positive(dn)) {
@@ -2050,6 +2118,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
 	ceph_fname_free_buffer(pinode, &tname);
 	ceph_fname_free_buffer(pinode, &oname);
 	iput(pinode);
+	kfree(long_snap_name);
 	dout("readdir_prepopulate done\n");
 	return err;
 }
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 5433e6050563..f001d57b0bd0 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -309,7 +309,8 @@ static int parse_reply_info_dir(void **p, void *end,
 
 static int parse_reply_info_lease(void **p, void *end,
 				  struct ceph_mds_reply_lease **lease,
-				  u64 features, u32 *altname_len, u8 **altname)
+				  u64 features, u32 *altname_len, u8 **altname,
+				  u8 *long_snap_name)
 {
 	u8 struct_v;
 	u32 struct_len;
@@ -338,14 +339,16 @@ static int parse_reply_info_lease(void **p, void *end,
 	*p += sizeof(**lease);
 
 	if (features == (u64)-1) {
+		*altname = NULL;
+		*altname_len = 0;
 		if (struct_v >= 2) {
 			ceph_decode_32_safe(p, end, *altname_len, bad);
 			ceph_decode_need(p, end, *altname_len, bad);
 			*altname = *p;
 			*p += *altname_len;
-		} else {
-			*altname = NULL;
-			*altname_len = 0;
+		}
+		if (struct_v >= 3) {
+			ceph_decode_8_safe(p, end, *long_snap_name, bad);
 		}
 		*p = end;
 	}
@@ -379,7 +382,8 @@ static int parse_reply_info_trace(void **p, void *end,
 		*p += info->dname_len;
 
 		err = parse_reply_info_lease(p, end, &info->dlease, features,
-					     &info->altname_len, &info->altname);
+					     &info->altname_len, &info->altname,
+					     &info->long_snap_name);
 		if (err < 0)
 			goto out_bad;
 	}
@@ -447,7 +451,8 @@ static int parse_reply_info_readdir(void **p, void *end,
 
 		/* dentry lease */
 		err = parse_reply_info_lease(p, end, &rde->lease, features,
-					     &rde->altname_len, &rde->altname);
+					     &rde->altname_len, &rde->altname,
+					     &rde->long_snap_name);
 		if (err)
 			goto out_bad;
 
@@ -2510,7 +2515,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for
 			spin_unlock(&cur->d_lock);
 		} else {
 			int len, ret;
-			char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
+			char buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX) + 20];
 
 			/*
 			 * Proactively copy name into buf, in case we need to present
@@ -2783,6 +2788,9 @@ static struct ceph_msg *create_request_message(struct ceph_mds_session *session,
 	if (test_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags))
 		len += sizeof(__le64);
 
+	/* extra chars '_' and '_${INO}' for long snap names */
+	len += 60;
+
 	msg = ceph_msg_new2(CEPH_MSG_CLIENT_REQUEST, len, 1, GFP_NOFS, false);
 	if (!msg) {
 		msg = ERR_PTR(-ENOMEM);
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 663d7754d57d..5068f85c7505 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -104,6 +104,7 @@ struct ceph_mds_reply_dir_entry {
 	struct ceph_mds_reply_lease   *lease;
 	struct ceph_mds_reply_info_in inode;
 	loff_t			      offset;
+	u8			      long_snap_name;
 };
 
 struct ceph_mds_reply_xattr {
@@ -129,6 +130,7 @@ struct ceph_mds_reply_info_parsed {
 	u32                           altname_len;
 	struct ceph_mds_reply_lease   *dlease;
 	struct ceph_mds_reply_xattr   xattr_info;
+	u8			      long_snap_name;
 
 	/* extra */
 	union {
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index e7c69ca5e289..f9ae5a1fe6e2 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -297,6 +297,7 @@ struct ceph_dentry_info {
 	unsigned long lease_renew_after, lease_renew_from;
 	unsigned long time;
 	u64 offset;
+	bool long_snap_name;
 };
 
 #define CEPH_DENTRY_REFERENCED		1
-- 
2.27.0