Re: [PATCH v4] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Wed, 2026-04-22 at 11:53 +0200, Ilya Dryomov wrote:
> On Fri, Apr 10, 2026 at 10:46 PM Viacheslav Dubeyko
> <Slava.Dubeyko@xxxxxxx> wrote:
> > 
> > On Fri, 2026-04-10 at 20:40 +0000, Viacheslav Dubeyko wrote:
> > > On Thu, 2026-04-09 at 18:09 +0000, Viacheslav Dubeyko wrote:
> > > > On Thu, 2026-04-09 at 10:39 +0800, Pengpeng Hou wrote:
> > > > > ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> > > > > name into the caller buffer and then, for long snapshot names, appends
> > > > > _<ino> with sprintf(p + elen, ...).
> > > > > 
> > > > > Some callers only provide NAME_MAX bytes. For long snapshot names, a
> > > > > large inode suffix can push the final encoded name past NAME_MAX even
> > > > > though the encrypted prefix stayed within the documented 240-byte
> > > > > budget.
> > > > > 
> > > > > Format the suffix into a small local buffer first and reject names
> > > > > whose suffix would exceed the caller's NAME_MAX output buffer.
> > > > > 
> > > > > Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> > > > > ---
> > > > > Changes since v3:
> > > > > - reject `elen > 240` explicitly instead of relying only on the earlier
> > > > >   `WARN_ON()`
> > > > > - rewrite the NAME_MAX bound check in terms of the final total length
> > > > >   instead of `NAME_MAX - prefix_len - elen`
> > > > > 
> > > > >  fs/ceph/crypto.c | 31 +++++++++++++++++++++++++++++--
> > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > > > > index f3de43ccb470..42e3fff34697 100644
> > > > > --- a/fs/ceph/crypto.c
> > > > > +++ b/fs/ceph/crypto.c
> > > > > @@ -15,6 +15,12 @@
> > > > >  #include "mds_client.h"
> > > > >  #include "crypto.h"
> > > > > 
> > > > > +/*
> > > > > + * Reserve room for '_' + decimal 64-bit inode number + trailing NUL.
> > > > > + * ceph_encode_encrypted_dname() copies only the visible suffix bytes.
> > > > > + */
> > > > > +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX       sizeof("_18446744073709551615")
> > > > > +
> > > > >  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
> > > > >  {
> > > > >   struct ceph_inode_info *ci = ceph_inode(inode);
> > > > > @@ -209,6 +215,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > >   struct inode *dir = parent;
> > > > >   char *p = buf;
> > > > >   u32 len;
> > > > > + int prefix_len = 0;
> > > > >   int name_len = elen;
> > > > >   int ret;
> > > > >   u8 *cryptbuf = NULL;
> > > > > @@ -219,6 +226,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > >           if (IS_ERR(dir))
> > > > >                   return PTR_ERR(dir);
> > > > >           p++; /* skip initial '_' */
> > > > > +         prefix_len = 1;
> > > > >   }
> > > > > 
> > > > >   if (!fscrypt_has_encryption_key(dir))
> > > > > @@ -271,8 +279,27 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
> > > > > 
> > > > >   /* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
> > > > >   WARN_ON(elen > 240);
> > > > > - if (dir != parent) // leading _ is already there; append _<inum>
> > > > > -         elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> > > > > + if (elen > 240) {
> > > > > +         elen = -ENAMETOOLONG;
> > > > > +         goto out;
> > > > > + }
> > > > > +
> > > > > + if (dir != parent) {
> > > > > +         int total_len;
> > > > > +         /* leading '_' is already there; append _<inum> */
> > > > > +         char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> > > > > +
> > > > > +         ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> > > > > +         total_len = prefix_len + elen + ret;
> > > > > +         if (total_len > NAME_MAX) {
> > > > > +                 elen = -ENAMETOOLONG;
> > > > > +                 goto out;
> > > > > +         }
> > > > > +
> > > > > +         memcpy(p + elen, suffix, ret);
> > > > > +         /* Include the leading '_' skipped by p. */
> > > > > +         elen = total_len;
> > > > > + }
> > > > > 
> > > > >  out:
> > > > >   kfree(cryptbuf);
> > > > 
> > > > Looks good.
> > > > 
> > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
> > > > 
> > > > Let me run xfstests for the patch to double check that everything is OK. I'll
> > > > share the result ASAP.
> > > > 
> > > 
> > > The xfstests run was successful. I don't see any issues with the patch.
> > > 
> > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
> > > 
> > > 
> > 
> > Applied on testing branch of CephFS kernel client git tree.
> 
> Hi Pengpeng, Slava,
> 
> This patch raised my attention because my understanding was that the
> entire CEPH_NOHASH_NAME_MAX + sha256() was put in place precisely to
> handle longer names nicely and make them fit into NAME_MAX-sized buffer.
> Simply rejecting longer names seemed to be in direct contradiction with
> that and yet the patch on its own was clearly merited given
> 
>  * (240 bytes is the maximum size allowed for snapshot names to take into
>  *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> 
> comment on CEPH_NOHASH_NAME_MAX definition.
> 
> I dug a bit deeper and started a discussion in [1].  The preliminary
> conclusion is that the 240 bytes assumption was a mistake -- somehow
> the minimum number of characters needed for <inum> ended up being used
> instead of the maximum.  CEPH_NOHASH_NAME_MAX value is likely incorrect
> and should have been smaller -- something along the lines of 174 -
> SHA256_DIGEST_SIZE instead of 180 - SHA256_DIGEST_SIZE.
> 

The limitation could be 240 or bigger one, but anyway we need to process this
limitation in proper way. And this patch has exactly this goal. Is 240 bytes
limitation your concern? As far as I can see, this patch doesn't introduce this
limitation. It was there before this modification. We can extend this limitation
anytime.

Thanks,
Slava.