Re: [PATCH 02/11] fbdev: Transfer video= option strings to caller; clarify ownership

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2166 bytes --]

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
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]