Re: [PATCH v2 000/101] fbdev: Fix memory leak in option parsing

[Geert Uytterhoeven]

Hi Thomas,

On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> Introduce struct option_iter and helpers to parse command-line
> options with comma-separated key-value pairs. Then convert fbdev
> drivers to the new interface. Fixes a memory leak in the parsing of
> the video= option.
>
> Before commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to
> caller; clarify ownership"), a call to fb_get_options() either
> returned an internal string or a duplicated string; hence ownership of
> the string's memory buffer was not well defined, but depended on how
> users specified the video= option on the kernel command line. For
> global settings, the caller owned the returned memory and for per-driver
> settings, fb_get_options() owned the memory. As calling drivers were
> unable to detect the case, they had no option but to leak the the memory.
>
> Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller;
> clarify ownership") changed semantics to caller-owned strings. Drivers
> still leaked the memory, but at least ownership was clear.

While I can find the actual patch[1], I cannot find this commit?
Where was it applied?

[1] https://lore.kernel.org/all/20230209135509.7786-3-tzimmermann@xxxxxxx

> This patchset fixes the memory leak and changes string ownership back
> to fb_get_options(). Patch 1 introduces struct option_iter and a few
> helpers. The interface takes an option string, such as video=, in the
> common form value1,key2:value2,value3 etc and returns the individual
> comma-separated pairs. Various modules use this pattern, so the code
> is located under lib/.
>
> Patches 2 to 100 go through fbdev drivers and convert them to the new
> interface. This often requires a number of cleanups. A driver would
> typically refer to the option string's video mode. Such strings are now
> copied to driver-allocated memory so that drivers don't refer directly
> to the option string's memory. The option iterator then replaces manual
> parsing loops based on strsep(","). All driver-allocated memory is
> released by removing the device or unloading the module.
>
> Patch 101 finally changes the ownership of the option string to be
> internal to fb_get_option(); thereby fixing the memory leak. The option
> iterator holds its own copy of the string and is not affected by the
> change.
>
> Most fbdev drivers only support to parse option strings if they are
> built-in. I assume that's because of the original fuzzy semantics of
> fb_get_options(). A later patchset could change the driver to respect
> video= settings in any configuration.
>
> v2:
>         * use kstrdup()/kfree() for video strings (Geert, Timur)
>         * fix iterator docs (Randy)
>         * update iterator interface

Thanks for the update, this looks much better!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds