linux-kernel - Re: [PATCH v2] Introduce support for Systems Management Driver over WMI for Dell Systems

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] [day] [month] [year] [list]

Date:   Mon, 21 Sep 2020 11:38:11 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Bharathi, Divya" <Divya.Bharathi@...l.com>,
        "Limonciello, Mario" <Mario.Limonciello@...l.com>,
        Divya Bharathi <divya27392@...il.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>
Subject: Re: [PATCH v2] Introduce support for Systems Management Driver over
 WMI for Dell Systems

Hi,

On 9/17/20 7:22 AM, Bharathi, Divya wrote:

<snip>

>>>>> +
>>>>> +/**
>>>>> + * exit_enum_attributes() - Clear all attribute data
>>>>> + * @kset: The kset to free
>>>>> + *
>>>>> + * Clears all data allocated for this group of attributes  **/ void
>>>>> +exit_enum_attributes(struct kset *kset) {
>>>>> +	struct kobject *pos, *next;
>>>>> +
>>>>> +	mutex_lock(&kset_mutex);
>>>>> +	list_for_each_entry_safe(pos, next, &kset->list, entry) {
>>>>> +		sysfs_remove_group(pos, &enumeration_attr_group);
>>>>> +	}
>>>>> +	mutex_unlock(&kset_mutex);
>>>>> +	mutex_lock(&enum_mutex);
>>>>> +	kfree(enumeration_data);
>>>>> +	mutex_unlock(&enum_mutex);
>>>>> +}
>>>>
>>>> Since there is now only 1 kset for the main dir, you are now calling
>>>> sysfs_remove_group 4 times (for all the different times) on each entry
>>>> in the attributes dir. I guess this may fail silently, but it still is
>>>> not good. So this needs to be fixed.
>>>>
>>>> The remarks to this file also apply to the:
>>>>
>>>> drivers/platform/x86/dell-wmi-int-attributes.c
>>>> drivers/platform/x86/dell-wmi-string-attributes.c
>>>>
>>>> files.
>>>>
> 
> Since we maintained 4 different attribute groups under 1 kset, each time
> respective attribute group will be removed. And once all groups are
> removed, kset is deleted.

sysfs_remove_group() just does a kernfs_remove_by_name() for each
attribute in the group.

Since the integer_, enumeration_ and string_ attr_group-s all
have e.g. a current_value attribute that means that current_value
will be removed 3 times and for the 2nd and 3th call
kernfs_remove_by_name() will fail with -ENOENT.

Currently neither kernfs_remove_by_name() nor sysfs_remove_group() print
an error message for this, but still it is not very clean.

Instead why not do this:

int populate_enum_data(union acpi_object *enumeration_obj, int instance_id,
                         struct kobject *attr_name_kobj)
{
         int retval = sysfs_create_group(attr_name_kobj, &enumeration_attr_group);
         int i, next_obj;

         if (retval)
                 goto out;

         mutex_lock(&wmi_priv.mutex);
	enumeration_data[instance_id].attr_name_kobj = attr_name_kobj;
	/* ^^^^^^^^^^^^^^^^ This line is new ^^^^^^^^^^^^^^^^^^^^^^^^*/
	...


void exit_enum_attributes(void)
{
         int i;

         for (i = 0; i < enumeration_instances_count; i++) {
		if (enumeration_data[instance_id].attr_name_kobj)
                         sysfs_remove_group(enumeration_data[instance_id].attr_name_kobj, &enumeration_attr_group);
	}

         kfree(enumeration_data);
}


That makes the teardown mirror the setup much more closely and as such is
a cleaner solution IMHO.

Regards,

Hans