| Message ID | 20230209135509.7786-3-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | drm,fbdev: Move video= option to drivers/video | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: > In fb_get_options(), always duplicate the returned option string and > transfer ownership of the memory to the function's caller. > > Until now, only the global option string got duplicated and transferred > to the caller; the per-driver options were owned by fb_get_options(). > In the end, it was impossible for the function's caller to detect if > it had to release the string's memory buffer. Hence, all calling drivers > leak the memory buffer. The leaks have existed ever since, but drivers > only call fb_get_option() once as part of module initialization. So the > amount of leaked memory is not significant. > > Fix the semantics of fb_get_option() by unconditionally transferring > ownership of the memory buffer to the caller. Later patches can resolve > the memory leaks in the fbdev drivers. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- [...] > + if (option) { > + if (options) > + *option = kstrdup(options, GFP_KERNEL); > + else > + *option = NULL; > + } > I know the old code wasn't checking if kstrdup() succeeded, but you should do it here and let the caller know. And same if (!options). So I guess the following check can be added (to be consistent with the rest of the code): if (!*option) retval = 1; > return retval; > } > -- > 2.39.1 Best regards, Javier
Hi Am 17.02.23 um 09:37 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > >> In fb_get_options(), always duplicate the returned option string and >> transfer ownership of the memory to the function's caller. >> >> Until now, only the global option string got duplicated and transferred >> to the caller; the per-driver options were owned by fb_get_options(). >> In the end, it was impossible for the function's caller to detect if >> it had to release the string's memory buffer. Hence, all calling drivers >> leak the memory buffer. The leaks have existed ever since, but drivers >> only call fb_get_option() once as part of module initialization. So the >> amount of leaked memory is not significant. >> >> Fix the semantics of fb_get_option() by unconditionally transferring >> ownership of the memory buffer to the caller. Later patches can resolve >> the memory leaks in the fbdev drivers. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- > > [...] > >> + if (option) { >> + if (options) >> + *option = kstrdup(options, GFP_KERNEL); >> + else >> + *option = NULL; >> + } >> > > I know the old code wasn't checking if kstrdup() succeeded, but you should Kstrdup uses kmalloc, which already warns about failed allocations. I think it's discouraged to warn again. (Wasn't there a warning in sparse or checkpatch?) So I'd rather leave it as is. > do it here and let the caller know. And same if (!options). So I guess the > following check can be added (to be consistent with the rest of the code): > > if (!*option) > retval = 1; Why is that needed for consistency? Retval is the state of the output: enabled or not. If there are no options, retval should be 0(=enabled). 1(=disabled) is only set by video=off or that ofonly thing. Best regards Thomas > >> return retval; >> } >> -- >> 2.39.1 > > Best regards, > Javier >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi > > Am 17.02.23 um 09:37 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >>> In fb_get_options(), always duplicate the returned option string and >>> transfer ownership of the memory to the function's caller. >>> >>> Until now, only the global option string got duplicated and transferred >>> to the caller; the per-driver options were owned by fb_get_options(). >>> In the end, it was impossible for the function's caller to detect if >>> it had to release the string's memory buffer. Hence, all calling drivers >>> leak the memory buffer. The leaks have existed ever since, but drivers >>> only call fb_get_option() once as part of module initialization. So the >>> amount of leaked memory is not significant. >>> >>> Fix the semantics of fb_get_option() by unconditionally transferring >>> ownership of the memory buffer to the caller. Later patches can resolve >>> the memory leaks in the fbdev drivers. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >> >> [...] >> >>> + if (option) { >>> + if (options) >>> + *option = kstrdup(options, GFP_KERNEL); >>> + else >>> + *option = NULL; >>> + } >>> >> >> I know the old code wasn't checking if kstrdup() succeeded, but you should > > Kstrdup uses kmalloc, which already warns about failed allocations. I > think it's discouraged to warn again. (Wasn't there a warning in sparse > or checkpatch?) So I'd rather leave it as is. > I didn't mean to warn but to return an error code. >> do it here and let the caller know. And same if (!options). So I guess the >> following check can be added (to be consistent with the rest of the code): >> >> if (!*option) >> retval = 1; > > Why is that needed for consistency? > > Retval is the state of the output: enabled or not. If there are no > options, retval should be 0(=enabled). 1(=disabled) is only set by > video=off or that ofonly thing. > Ah, I see. I misundertood what retval was about. Forget this comment then. Maybe while you are there could have another patch to document the return value in the fb_get_options() kernel-doc? And this patch looks good to me too after your explanations. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards, Javier
diff --git a/drivers/video/fbdev/core/fb_cmdline.c b/drivers/video/fbdev/core/fb_cmdline.c index 6792010d6716..702b00b71870 100644 --- a/drivers/video/fbdev/core/fb_cmdline.c +++ b/drivers/video/fbdev/core/fb_cmdline.c @@ -30,13 +30,17 @@ EXPORT_SYMBOL_GPL(fb_mode_option); * (video=<name>:<options>) * @option: the option will be stored here * + * The caller owns the string returned in @option and is + * responsible for releasing the memory. + * * NOTE: Needed to maintain backwards compatibility */ int fb_get_options(const char *name, char **option) { - char *opt, *options = NULL; + const char *options = NULL; int retval = 0; int name_len = strlen(name), i; + char *opt; if (name_len && ofonly && strncmp(name, "offb", 4)) retval = 1; @@ -55,12 +59,16 @@ int fb_get_options(const char *name, char **option) } /* No match, pass global option */ if (!options && option && fb_mode_option) - options = kstrdup(fb_mode_option, GFP_KERNEL); + options = fb_mode_option; if (options && !strncmp(options, "off", 3)) retval = 1; - if (option) - *option = options; + if (option) { + if (options) + *option = kstrdup(options, GFP_KERNEL); + else + *option = NULL; + } return retval; }
In fb_get_options(), always duplicate the returned option string and transfer ownership of the memory to the function's caller. Until now, only the global option string got duplicated and transferred to the caller; the per-driver options were owned by fb_get_options(). In the end, it was impossible for the function's caller to detect if it had to release the string's memory buffer. Hence, all calling drivers leak the memory buffer. The leaks have existed ever since, but drivers only call fb_get_option() once as part of module initialization. So the amount of leaked memory is not significant. Fix the semantics of fb_get_option() by unconditionally transferring ownership of the memory buffer to the caller. Later patches can resolve the memory leaks in the fbdev drivers. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/video/fbdev/core/fb_cmdline.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)