Re: [PATCH] fs: do not cache a symlink length that disagrees with the string

On Mon, Aug 17, 2026 at 10:17 AM Narek Jilavyan <[email protected]> wrote:
>
> inode_set_cached_link() stores a caller-supplied length in i_linklen and
> sets IOP_CACHED_LINK. vfs_readlink() then uses that length directly:
>
>         if (inode->i_opflags & IOP_CACHED_LINK)
>                 return readlink_copy(buffer, buflen, inode->i_link,
>                                      inode->i_linklen);
>
> and readlink_copy() clamps only against the user buffer, not against
> the string, so a length larger than the symlink body becomes a
> copy_to_user() of adjacent kernel memory - reachable by any process
> calling readlink() on such a symlink.
>
> The only thing standing behind the invariant is
>
>         VFS_WARN_ON_INODE(strlen(link) != linklen, inode);
>
> which expands to BUILD_BUG_ON_INVALID() unless CONFIG_DEBUG_VFS is set.
> On a production kernel it type-checks the expression and evaluates
> nothing, so the value is stored unvalidated.
>
> All four in-tree callers are correct today, and notably the two whose
> length comes from on-disk metadata (fs/ext4/inode.c, fs/erofs/inode.c)
> both re-derive it and reject the inode rather than relying on this
> helper.  The API should not require that of the next caller.
>

I'm confused why you don't expect filesystems to guarantee correct
value here, but I can agree issuing the strlen is not a big deal
during inode setup and I'm not going to insist on NOT having it in
prod kernels.

However, if going that route, I think the proposed WARN_ON is too terse.

inode_set_cached_link() at some point had the following:
       testlen = strlen(link);
       if (testlen != linklen) {
               WARN_ONCE(1, "bad length passed for symlink [%s] (got
%d, expected %d)",
                         link, linklen, testlen);
               linklen = testlen;
       }

As in, it used the correct value to fix up the caller.

I think it is ok to refrain from caching.

The important bit is the extra information as to what's going on,
namely what's the len disparity. I guess the message would be nicer if
the it also printed the filesystem name. You can borrow code from
dump_inode to do it.


> Validate unconditionally and fail safe: if the length disagrees, warn
> and leave IOP_CACHED_LINK clear.  i_linklen has exactly one reader in
> the tree and it is gated on that flag, and vfs_readlink() falls back to
> i_link with a strlen() of its own, so the inode degrades to the
> behaviour that predates the cached length instead of disclosing memory.
>
> The check runs once per symlink inode setup, not once per readlink(),
> which is what the cache was for.
>
> Fixes: ea3821990719 ("vfs: support caching symlink lengths in inodes")
> Signed-off-by: Narek Jilavyan <[email protected]>
> ---
>  include/linux/fs.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 50ce731a2..e1d8f2614 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -946,9 +946,19 @@ static inline void inode_state_replace(struct inode *inode,
>
>  static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen)
>  {
> -       VFS_WARN_ON_INODE(strlen(link) != linklen, inode);
>         VFS_WARN_ON_INODE(inode->i_opflags & IOP_CACHED_LINK, inode);
>         inode->i_link = link;
> +
> +       /*
> +        * i_linklen is used as a copy_to_user() length by vfs_readlink(), so it
> +        * must not be taken on trust. If it disagrees with the string, leave
> +        * IOP_CACHED_LINK clear: vfs_readlink() then falls back to i_link and
> +        * recomputes the length with strlen(), which is what it did before the
> +        * cached length was introduced.
> +        */
> +       if (WARN_ON_ONCE(strlen(link) != linklen))
> +               return;
> +
>         inode->i_linklen = linklen;
>         inode->i_opflags |= IOP_CACHED_LINK;
>  }
> --
> 2.43.0
>