Re: [PATCH v2] cache the string generated by reading /proc/filesystems

[Mateusz Guzik]

On Thu, Apr 23, 2026 at 12:07 AM Andreas Dilger <adilger@xxxxxxxxx> wrote:
>
> On Apr 22, 2026, at 12:17, Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
> > +     new = kmalloc(offsetof(struct file_systems_string, string) + newlen + 1,
> > +                   GFP_KERNEL);
> > +     if (!new)
> > +             return -ENOMEM;
>
> This is introducing an error case where there was none before, in a
> critical system file.  It would also be possible to fall back to
> seq_printf() instead of sprinft() if no buffer is available.
>

Fair, should be easy to do.

> > +     /*
> > +      * Populate the string.
> > +      *
> > +      * We know we have just enough space because we calculated the right
> > +      * size the previous time we had the lock and confirmed the list has
> > +      * not changed after reacquiring it.
> > +      */
>
> It doesn't seem prudent to use raw sprintf() in a loop and justify this in
> a comment, when the actual string generation is not performance critical.
> This could use scnprintf() and be "safer" even if it "should just work" (tm).
>

I don't get how that's supposed help anything. I still have to call
the routine in a loop, except this time I have to keep recalculating
remaining size by hand on top of it.

What this really wants is a string library which handles easy things
like this one, FreeBSD has something like this in
https://man.freebsd.org/cgi/man.cgi?sbuf(9) , but I certainly can't be
bothered to work on an equivalent or porting it over.

If one is to "overflow-proof this" with total disregard for
performance, a series of strlcats would do it.

> > +      usedlen = 0;
> >       tmp = file_systems;
> >       while (tmp) {
> > -             seq_printf(m, "%s\t%s\n",
> > +             usedlen += sprintf(&new->string[usedlen], "%s\t%s\n",
> >                       (tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
> >                       tmp->name);
> >               tmp = tmp->next;
> >       }
> > -     read_unlock(&file_systems_lock);
> > +     BUG_ON(new->len != strlen(new->string));
>
> Adding a BUG_ON() here seems gratuitous, when there are lots of other
> options to handle this case, especially if sprintf() didn't just corrupt
> memory for some reason.  It could allocate a larger buffer and retry,
> or in the worst case return an error instead of crashing the system.
>

Well nobody is supposed to get the splat, except maybe someone messing
with how the string is generated, so I don't really see your point --
it's purely a bug-catching measure.

However, it can still just print a warn and fallback to the current
pointer chasing.