linux-kernel - Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160224082602.GC19954@box2.japko.eu>
Date:	Wed, 24 Feb 2016 09:26:03 +0100
From:	Krzysztof Adamski <krzysztof.adamski@...to.com>
To:	Jon Hunter <jonathanh@...dia.com>
Cc:	Mark Brown <broonie@...nel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: core: fix error path of
 regulator_ena_gpio_free

On Tue, Feb 23, 2016 at 03:18:59PM +0000, Jon Hunter wrote:
>
>On 23/02/16 14:47, Krzysztof Adamski wrote:
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@...to.com>
>> Reported-by: Jon Hunter <jonathanh@...dia.com>
>
>Nit ... I think that order of the above should be reversed.
>

Couldn't find any reference stating proper order of those tags and 
briefly looking at other commit messages shows this order as quite 
common.

>> ---
>>  drivers/regulator/core.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index 6ee9ba4..d1e7859 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>  		if (ret != 0) {
>>  			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
>>  				 config->ena_gpio, ret);
>> -			goto wash;
>> +			goto clean;
>>  		}
>>  	}
>>
>> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>
>>  	ret = set_machine_constraints(rdev, constraints);
>>  	if (ret < 0)
>> -		goto scrub;
>> +		goto wash;
>>
>>  	if (init_data && init_data->supply_regulator)
>>  		rdev->supply_name = init_data->supply_regulator;
>> @@ -3972,10 +3972,8 @@ out:
>>  unset_supplies:
>>  	unset_regulator_supplies(rdev);
>>
>> -scrub:
>> -	regulator_ena_gpio_free(rdev);
>> -
>>  wash:
>> +	regulator_ena_gpio_free(rdev);
>>  	device_unregister(&rdev->dev);
>>  	/* device core frees rdev */
>>  	rdev = ERR_PTR(ret);
>
>What about the case where device_register() fails? I think you still
>call clean and so you will leak the gpio?
>
>Jon
>
True. I couldn't find anything more clever than calling 
regulator_ena_gpio_free() in two paths like in an upcomming v2. Putting 
it inside of regulator_dev_release() won't entirely fix the problem 
either as this won't be called in this particular case 
(device_register() fail). I personally still prefer calling 
regulator_ena_gpio_free() inside of regulator_register insted of 
deffering it to regulator_dev_release() as it seems to be clearer to me.

Best regards,
Krzysztof Adamski