On Tue, Feb 04, 2025 at 10:32:07PM +0100, Mateusz Guzik wrote: > This costs a strlen() call when instatianating a symlink. > > Preferably it would be hidden behind VFS_WARN_ON (or compatible), but > there is no such facility at the moment. With the facility in place the > call can be patched out in production kernels. > > In the meantime, since the cost is being paid unconditionally, use the > result to a fixup the bad caller. > > This is not expected to persist in the long run (tm). > > Sample splat: > bad length passed for symlink [/tmp/syz-imagegen43743633/file0/file0] (got 131109, expected 37) > [rest of WARN blurp goes here] > > Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx> > --- > > This has a side effect of working around the panic reported in: > https://lore.kernel.org/all/67a1e1f4.050a0220.163cdc.0063.GAE@xxxxxxxxxx/ > > I'm confident this merely exposed a bug in ext4, see: > https://lore.kernel.org/all/CAGudoHEv+Diti3r0x9VmF5ixgRVKk4trYnX_skVJNkQoTMaDHg@xxxxxxxxxxxxxx/#t Yes, ext4 should not continue to load a fast symlink where i_size > sizeof(i_data). I'm a little surprised that nd_terminate_link exists to paper over this situation, but the commit adding it (035146851cfa2fe) is from 2008 when we were all young and naïve. But yes, would be nice to have a CONFIG_VFS_DEBUG behind which to hide assertions so that insufficient validation in ext4 will burble up through Ted's test infrastructure. ;) That said, I just saw this in ext4_get_link: nd_terminate_link(bh->b_data, inode->i_size, inode->i_sb->s_blocksize - 1); Which is writing into a buffer head without a transaction? Granted symlink targets in ext4 are written out to disk with the null terminator so I guess that's not a big deal. --D > > Nonethelss, should help catch future problems. > > include/linux/fs.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index be3ad155ec9f..1437a3323731 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -791,6 +791,19 @@ struct inode { > > static inline void inode_set_cached_link(struct inode *inode, char *link, int linklen) > { > + int testlen; > + > + /* > + * TODO: patch it into a debug-only check if relevant macros show up. > + * In the meantime, since we are suffering strlen even on production kernels > + * to find the right length, do a fixup if the wrong value got passed. > + */ > + testlen = strlen(link); > + if (testlen != linklen) { > + WARN_ONCE(1, "bad length passed for symlink [%s] (got %d, expected %d)", > + link, linklen, testlen); > + linklen = testlen; > + } > inode->i_link = link; > inode->i_linklen = linklen; > inode->i_opflags |= IOP_CACHED_LINK; > -- > 2.43.0 > >
