| Date | Fri, 10 Mar 2023 13:44:30 +0100 | Subject | Re: [PATCH v2 000/101] fbdev: Fix memory leak in option parsing | From | Thomas Zimmermann <> |
| |
Hi Geert
Am 10.03.23 um 09:24 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@suse.de> 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?
It's currently in drm-misc-next.
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=73ce73c30ba9ae4d90fdfad7ebe9104001d5d851
Best regards Thomas
> > [1] https://lore.kernel.org/all/20230209135509.7786-3-tzimmermann@suse.de > >> 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 >
-- 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 [unhandled content-type:application/pgp-signature] |