Re: [PATCH 00/99] fbdev: Fix memory leak in option parsing

[Thomas Zimmermann]

Hi Geert

Am 07.03.23 um 08:53 schrieb Geert Uytterhoeven:
> Hi Thomas,
>
> On Mon, Mar 6, 2023 at 5:00 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, the memory was leaked.
> >
> > Commit 73ce73c30ba9 ("fbdev: Transfer video= option strings to caller;
> > clarify ownership") changed sematics to caller-owned strings. Drivers
> > still leaked the memory, but at least ownership was clear.
> >
> > 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 individial
> > comma-separated pairs. Various modules use this pattern, so the code
> > is located under lib/.
> >
> > Patches 2 to 98 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(",").
>
> Thanks for your series!
>
> Unfortunately I cannot say I'm thrilled about this: you are replacing
> a single small dynamic memory leak by 36 larger static memory leaks.

That's fair enough.

> Am I missing something?

The current size of the videomode buffers is ridiculously large. I just 
needed something that could hold the string. A long mode description 
might look like

  1920x1080MR-32@120ime

which has 21 characters. 32-byte buffers would probably be more than enough.

I think it should also be possible to do a simple kstrdup() on the given 
videomode string and free the copy in the module's _fini function.

That also brings up the question of these MODULE ifdefs. Almost all of 
the fbdev drivers only parse the command-line option if they are not 
build as a module. Do you know why? Because of the awkward semantics of 
the old fb_get_options()?  I think this should be changed so that they 
always respect the video= parameter.

Best regards
Thomas

> Gr{oetje,eeting}s,
>
>                          Geert

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature