| Message ID | 20230309160201.5163-62-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | fbdev: Fix memory leak in option parsing | expand |
Hi Thomas, On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Assume that the driver does not own the option string or its substrings > and hence duplicate the option string for the video mode. Allocate the > copy's memory with kstrdup() and free it in the module's exit function. > > Done in preparation of switching the driver to struct option_iter and > constifying the option string. > > v2: > * replace static memory with kstrdup()/kfree() (Geert) > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Thanks for the upodate! > --- a/drivers/video/fbdev/ps3fb.c > +++ b/drivers/video/fbdev/ps3fb.c > @@ -260,6 +260,7 @@ static const struct fb_videomode ps3fb_modedb[] = { > static int ps3fb_mode; > module_param(ps3fb_mode, int, 0); > > +static char *mode_option_buf; Do you really need this variable? It contains the same value as mode_option below. This is a common pattern in several patches. > static char *mode_option; > > static int ps3fb_cmp_mode(const struct fb_videomode *vmode, > @@ -1276,8 +1277,11 @@ static int __init ps3fb_setup(void) > continue; > if (!strncmp(this_opt, "mode:", 5)) > ps3fb_mode = simple_strtoul(this_opt + 5, NULL, 0); > - else > - mode_option = this_opt; > + else { > + kfree(mode_option_buf); > + mode_option_buf = kstrdup(this_opt, GFP_KERNEL); // ignore errors > + mode_option = mode_option_buf; > + } > } > return 0; > } > @@ -1294,6 +1298,7 @@ static void __exit ps3fb_exit(void) > { > pr_debug(" -> %s:%d\n", __func__, __LINE__); > ps3_system_bus_driver_unregister(&ps3fb_driver); > + kfree(mode_option_buf); > pr_debug(" <- %s:%d\n", __func__, __LINE__); > } Gr{oetje,eeting}s, Geert
Hi Geert Am 10.03.23 um 09:18 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Thu, Mar 9, 2023 at 5:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Assume that the driver does not own the option string or its substrings >> and hence duplicate the option string for the video mode. Allocate the >> copy's memory with kstrdup() and free it in the module's exit function. >> >> Done in preparation of switching the driver to struct option_iter and >> constifying the option string. >> >> v2: >> * replace static memory with kstrdup()/kfree() (Geert) >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Thanks for the upodate! > >> --- a/drivers/video/fbdev/ps3fb.c >> +++ b/drivers/video/fbdev/ps3fb.c >> @@ -260,6 +260,7 @@ static const struct fb_videomode ps3fb_modedb[] = { >> static int ps3fb_mode; >> module_param(ps3fb_mode, int, 0); >> >> +static char *mode_option_buf; > > Do you really need this variable? It contains the same value > as mode_option below. > This is a common pattern in several patches. In most cases, the extra variable is necessary. mode_option, or a similar variable, is often also used to store module parameters or is initialized from a static string. In those cases we obviously cannot free the memory. Hence the additional variable for the freeing the allocation. I reviewed the patches and it looks like kyrofb, ps3fb, and sm712fb might be able to work without the _buf variable. I'll try to update those. Best regards Thomas > >> static char *mode_option; >> >> static int ps3fb_cmp_mode(const struct fb_videomode *vmode, >> @@ -1276,8 +1277,11 @@ static int __init ps3fb_setup(void) >> continue; >> if (!strncmp(this_opt, "mode:", 5)) >> ps3fb_mode = simple_strtoul(this_opt + 5, NULL, 0); >> - else >> - mode_option = this_opt; >> + else { >> + kfree(mode_option_buf); >> + mode_option_buf = kstrdup(this_opt, GFP_KERNEL); // ignore errors >> + mode_option = mode_option_buf; >> + } >> } >> return 0; >> } >> @@ -1294,6 +1298,7 @@ static void __exit ps3fb_exit(void) >> { >> pr_debug(" -> %s:%d\n", __func__, __LINE__); >> ps3_system_bus_driver_unregister(&ps3fb_driver); >> + kfree(mode_option_buf); >> pr_debug(" <- %s:%d\n", __func__, __LINE__); >> } > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/video/fbdev/ps3fb.c b/drivers/video/fbdev/ps3fb.c index 2fe08b67eda7..d67ef2701b18 100644 --- a/drivers/video/fbdev/ps3fb.c +++ b/drivers/video/fbdev/ps3fb.c @@ -260,6 +260,7 @@ static const struct fb_videomode ps3fb_modedb[] = { static int ps3fb_mode; module_param(ps3fb_mode, int, 0); +static char *mode_option_buf; static char *mode_option; static int ps3fb_cmp_mode(const struct fb_videomode *vmode, @@ -1276,8 +1277,11 @@ static int __init ps3fb_setup(void) continue; if (!strncmp(this_opt, "mode:", 5)) ps3fb_mode = simple_strtoul(this_opt + 5, NULL, 0); - else - mode_option = this_opt; + else { + kfree(mode_option_buf); + mode_option_buf = kstrdup(this_opt, GFP_KERNEL); // ignore errors + mode_option = mode_option_buf; + } } return 0; } @@ -1294,6 +1298,7 @@ static void __exit ps3fb_exit(void) { pr_debug(" -> %s:%d\n", __func__, __LINE__); ps3_system_bus_driver_unregister(&ps3fb_driver); + kfree(mode_option_buf); pr_debug(" <- %s:%d\n", __func__, __LINE__); }
Assume that the driver does not own the option string or its substrings and hence duplicate the option string for the video mode. Allocate the copy's memory with kstrdup() and free it in the module's exit function. Done in preparation of switching the driver to struct option_iter and constifying the option string. v2: * replace static memory with kstrdup()/kfree() (Geert) Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/video/fbdev/ps3fb.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)