On Wed, Feb 5, 2025 at 5:12 PM Jan Kara <jack@xxxxxxx> wrote: > > On Tue 04-02-25 22:32:07, 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> > > Yeah, it looks a bit pointless to pass the length in only to compare it > against strlen(). But as a quick fix until we figure out something more > clever it's fine I guess. > There is some dayjob stuff I need to take care of. Once I'm both out from under the ruble *and* bored enough, I'll post provisional support for VFS_WARN_ON and VFS_BUG_ON macros. But that's for -next. > Feel free to add: > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > Honza > > > --- > > > > 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 > > > > 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 > > > -- > Jan Kara <jack@xxxxxxxx> > SUSE Labs, CR -- Mateusz Guzik <mjguzik gmail.com>
