Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Luis Henriques <luis@igalia.com>]

[ Dropping my previous email from the CC list ]

On Fri, Feb 14 2025, Jeff Layton wrote:

> On Fri, 2025-02-14 at 03:28 +0000, Al Viro wrote:
>> On Fri, Feb 14, 2025 at 02:47:56AM +0000, Al Viro wrote:
>> 
>> [snip]
>> 
>> > Am I missing something subtle here?  Can elen be non-positive at that point?
>> 
>> Another fun question: for dentries with name of form _<something>_<inumber>
>> we end up looking at fscrypt_has_encryption_key() not for the parent,
>> but for inode with inumber encoded in dentry name.  Fair enough, but...
>> what happens if we run into such dentry in ceph_mdsc_build_path()?
>> 
>> There the call of ceph_encode_encrypted_fname() is under
>> 	if (fscrypt_has_encryption_key(d_inode(parent)))
>> 
>> Do we need the keys for both?
>> 
>
> That sounds like a bug, but I don't fully recall whether snapshots have
> a special case here for some reason. Let me rephrase Al's question:
>
> If I have a snapshot dir that is prefixed with '_', why does it use a
> different filename encryption key than other snapshot dirs that don't
> start with that character?
>
> My guess here is that this code ought not overwrite "dir" with the
> result of parse_longname(), but I don't recall the significance of a
> snapshot name that starts with '_'.

This bit I _think_ I remember.  Imagine this tree structure:

   /mnt/ceph
   |-- mydir1
   |-- mydir2

If you create a snapshot in /mnt/ceph:

  mkdir /mnt/ceph/.snap/my-snapshot

you'll see this:

   /mnt/ceph
   |-- .snap
   |     |-- my-snapshot
   |-- mydir1
   |     |-- _my-snapshot_1099511627782
   |-- mydir2
         |-- _my-snapshot_1099511627782

('1099511627782' is the inode number where the snapshot was created.)

So, IIRC, when encrypting the snapshot name (the "my-snapshot" string),
you'll use key from the original inode.  That's why we need to handle
snapshot names starting with '_' differently.  And why we have a
customized base64 encoding function.

Cheers,
-- 
Luís


>         /* Handle the special case of snapshot names that start with '_' */
>         if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
>             (iname.name[0] == '_')) {
>                 dir = parse_longname(parent, iname.name, &name_len);
>                 if (IS_ERR(dir))
>                         return PTR_ERR(dir);
>                 iname.name++; /* skip initial '_' */
>         }
>         iname.len = name_len;
>
>         if (!fscrypt_has_encryption_key(dir)) {
>                 memcpy(buf, d_name->name, d_name->len);
>                 elen = d_name->len;
>                 goto out;
>         }
>
>
> -- 
> Jeff Layton <jlayton@kernel.org>
>