[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2471a906-1211-4784-ac49-24f5ff313cc4@oracle.com>
Date: Fri, 10 Nov 2023 19:24:01 +0530
From: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Jorge Lopez <jorge.lopez2@...com>,
Hans de Goede <hdegoede@...hat.com>,
Mark Gross <markgross@...nel.org>,
Thomas Weißschuh <linux@...ssschuh.net>,
platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, dan.carpenter@...aro.org,
kernel-janitors@...r.kernel.org, error27@...il.com,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] platform/x86: hp-bioscfg: Fix error handling in
hp_add_other_attributes()
Hi Ilpo,
On 10/11/23 6:45 pm, Ilpo Järvinen wrote:
> On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
>
>> 1. acpi_object *obj is unused in this function, so delete it, also
>> delete a unnecessary kfree(obj);
>> 2. Fix a memory leak of 'attr_name_kobj' in the error handling path.
>> 3. When kobject_init_and_add() fails on every subsequent error path call
>> kobject_put() to cleanup.
>>
>> Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
>> Reported-by: kernel test robot <lkp@...el.com>
>> Reported-by: Dan Carpenter <error27@...il.com>
>> Closes: https://lore.kernel.org/r/202309201412.on0VXJGo-lkp@intel.com/
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
>> ---
>> This is only compile tested, based on static analysis.
>> ---
>> drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 14 ++++++--------
>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
>> index 5798b49ddaba..b28e52b64690 100644
>> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
>> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
>> @@ -588,7 +588,6 @@ static void release_attributes_data(void)
>> static int hp_add_other_attributes(int attr_type)
>> {
>> struct kobject *attr_name_kobj;
>> - union acpi_object *obj = NULL;
>> int ret;
>> char *attr_name;
>>
>> @@ -596,8 +595,8 @@ static int hp_add_other_attributes(int attr_type)
>>
>> attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
>> if (!attr_name_kobj) {
>> - ret = -ENOMEM;
>> - goto err_other_attr_init;
>> + mutex_unlock(&bioscfg_drv.mutex);
>
> I don't understand why this has to be inside the mutex at all, cannot
> you just move it outside of the mutex and then there's no need to unlock?
>
Oh right, I think this allocation can be done without a lock.
Will move this down in my v2.
Thanks for the suggestion.
Regards,
Harshit
>> + return -ENOMEM;
>> }
>>
>> /* Check if attribute type is supported */
>> @@ -614,15 +613,15 @@ static int hp_add_other_attributes(int attr_type)
>>
>> default:
>> pr_err("Error: Unknown attr_type: %d\n", attr_type);
>> - ret = -EINVAL;
>> - goto err_other_attr_init;
>> + kfree(attr_name_kobj);
>> + mutex_unlock(&bioscfg_drv.mutex);
>> + return -EINVAL;
>
> Add a new label for unlock and goto to it instead.
>
>> }
>>
>> ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype,
>> NULL, "%s", attr_name);
>> if (ret) {
>> pr_err("Error encountered [%d]\n", ret);
>> - kobject_put(attr_name_kobj);
>> goto err_other_attr_init;
>> }
>>
>> @@ -647,10 +646,9 @@ static int hp_add_other_attributes(int attr_type)
>>
>> mutex_unlock(&bioscfg_drv.mutex);
>> return 0;
>> -
>> err_other_attr_init:
>> + kobject_put(attr_name_kobj);
>
> unlock:
>
>> mutex_unlock(&bioscfg_drv.mutex);
>> - kfree(obj);
>> return ret;
>> }
>>
>>
>
>
Powered by blists - more mailing lists