diff mbox series

[v2,061/101] fbdev/ps3fb: Duplicate video-mode option string

Message ID 20230309160201.5163-62-tzimmermann@suse.de
State New
Headers
Series fbdev: Fix memory leak in option parsing |

Commit Message

Thomas Zimmermann March 9, 2023, 4:01 p.m. UTC 
  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(-)

Comments

Geert Uytterhoeven March 10, 2023, 8:18 a.m. UTC | #1 
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
Thomas Zimmermann March 10, 2023, 12:21 p.m. UTC | #2 
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
> 

-- 
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
diff mbox series

Patch

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__);
 }