lkml.org 
[lkml]   [2020]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5] Introduce support for Systems Management Driver over WMI for Dell Systems
From
Date
HI,

On 10/6/20 10:46 AM, Bharathi, Divya wrote:
> Sorry for another mail on same patch. I missed to add one response on
> previous comments

No problem.

> <snip>
>
>>> +/**
>>> + * init_bios_attributes() - Initialize all attributes for a type
>>> + * @attr_type: The attribute type to initialize
>>> + * @guid: The WMI GUID associated with this type to initialize
>>> + *
>>> + * Initialiaze all 4 types of attributes enumeration, integer, string and
>> password object.
>>> + * Populates each attrbute typ's respective properties under sysfs files
>>> + **/
>>> +static int init_bios_attributes(int attr_type, const char *guid)
>>> +{
>>> + struct kobject *attr_name_kobj; //individual attribute names
>>> + union acpi_object *obj = NULL;
>>> + union acpi_object *elements;
>>> + struct kset *tmp_set;
>>> +
>>> + /* instance_id needs to be reset for each type GUID
>>> + * also, instance IDs are unique within GUID but not across
>>> + */
>>> + int instance_id = 0;
>>> + int retval = 0;
>>> +
>>> + retval = alloc_attributes_data(attr_type);
>>> + if (retval)
>>> + return retval;
>>> + /* need to use specific instance_id and guid combination to get right
>> data */
>>> + obj = get_wmiobj_pointer(instance_id, guid);
>>> + if (!obj)
>>> + return -ENODEV;
>>> + elements = obj->package.elements;
>>> +
>>> + mutex_lock(&wmi_priv.mutex);
>>> + while (elements) {
>>> + /* sanity checking */
>>> + if (strlen(elements[ATTR_NAME].string.pointer) == 0) {
>>> + pr_debug("empty attribute found\n");
>>> + goto nextobj;
>>> + }
>>> + if (attr_type == PO)
>>> + tmp_set = wmi_priv.authentication_dir_kset;
>>> + else
>>> + tmp_set = wmi_priv.main_dir_kset;
>>> +
>>> + if (kset_find_obj(tmp_set,
>> elements[ATTR_NAME].string.pointer)) {
>>> + pr_debug("duplicate attribute name found - %s\n",
>>> + elements[ATTR_NAME].string.pointer);
>>> + goto nextobj;
>>> + }
>>> +
>>> + /* build attribute */
>>> + attr_name_kobj = kzalloc(sizeof(*attr_name_kobj),
>> GFP_KERNEL);
>>> + if (!attr_name_kobj)
>>> + goto err_attr_init;
>>> +
>>> + attr_name_kobj->kset = tmp_set;
>>> +
>>> + retval = kobject_init_and_add(attr_name_kobj,
>> &attr_name_ktype, NULL, "%s",
>>> +
>> elements[ATTR_NAME].string.pointer);
>>> + if (retval) {
>>> + kobject_put(attr_name_kobj);
>>> + goto err_attr_init;
>>> + }
>>> +
>>> + /* enumerate all of this attribute */
>>> + switch (attr_type) {
>>> + case ENUM:
>>> + retval = populate_enum_data(elements, instance_id,
>> attr_name_kobj);
>>> + break;
>>> + case INT:
>>> + retval = populate_int_data(elements, instance_id,
>> attr_name_kobj);
>>> + break;
>>> + case STR:
>>> + retval = populate_str_data(elements, instance_id,
>> attr_name_kobj);
>>> + break;
>>> + case PO:
>>> + retval = populate_po_data(elements, instance_id,
>> attr_name_kobj);
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>
>> The show functions don't take the mutex and can be called as soon as
>> kobject_init_and_add() is called, so it would be better to first populate the
>> data for the current instance_id and only then call kobject_init_and_add()
>>
>
> Populate functions called here for each type of attribute uses
> attribute_koject which helps in attribute group cleanup.

Good point, and we do allocate the data before creating the kobjects,
so if a user manages to hit the race (which almost certainly would have
to be done intentionally) then the read would just result in an empty
string (rather then say a null pointer dereference oops).

So lets just keep this as is.

Regards,

Hans

\
 
 \ /
  Last update: 2020-10-09 09:59    [W:0.183 / U:3.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site