Re: [RFC/BUG] Use a bounce buffer for mds client decryption

[Sam Edwards]

On Wed, Apr 15, 2026 at 10:16 AM Alex Markuze <amarkuze@xxxxxxxxxx> wrote:
>
> Hi Sam,
>
> Nice find. The diagnosis is correct and I like the bounce buffer approach.
>
> A few things for v2:
>
> 1. Typo in the comment: "fscript_str" should be "fscrypt_str".
>
> 2. The assignment-in-if is nice but limits readability:
>
> ```c
> if ((err = ceph_fname_alloc_buffer(inode, &bname)))
> ```
>
> Split it:
>
> ```c
> err = ceph_fname_alloc_buffer(inode, &bname);
> if (err)
>         goto out_bad;
> ```
>
> 3. The `BUG_ON(oname.len > bname.len)` should be a `WARN_ON_ONCE` with
> an error return instead. New `BUG_ON`s in data-dependent paths are
> unlikely to pass review.
>
> 4. The copy-back block is correct but not obvious at a glance. A short
> comment explaining why you need to copy back out of the bounce buffer
> (rde->name must point into the long-lived front buffer) would help.

Hi Alex,

ACK to all of the above. Which return code is best for the
WARN_ON_ONCE violation?

> 5. `parse_reply_info_trace()` has the same latent bug. It stores
> `info->dname` and `info->altname` as raw pointers into the front
> buffer. When the LOOKUPNAME path in `ceph_fill_inode()` later passes
> `fname.ctext = rinfo->altname` to `ceph_fname_to_usr()`, that pointer
> hits `sg_init_one()` in `fname_decrypt()` as the source scatterlist.
> The output buffer is separately allocated so it's fine, but the input is not.

I'm happy to bundle a fix for this other problem, but I'd need a way
to observe+validate it. Do you have a reproducer, bug report, or at
least a call trace for this?

The callers of ceph_fill_inode() that I can see are
ceph_finish_async_create(), ceph_fill_trace(), and
ceph_readdir_prepopulate(), none of which are (callees of)
parse_reply_info_trace(). Is the culprit handle_reply() calling
parse_reply_info_trace(), then ceph_fill_trace() while passing the bad
buffer inside `req`? If so, is handle_reply() responsible for the
bounce buffer's lifetime, or should it be handled by
ceph_fill_trace()?

> 6. Also, let's add an `unlikely` on the is_vmalloc_addr

Did you mean `if (bname.name)`?

Thank you greatly,
Sam