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 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