linux-kernel - Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

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]

Date:   Tue, 29 Mar 2022 16:37:02 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Eugene Shalygin <eugene.shalygin@...il.com>
Cc:     darcagn@...tonmail.com, Jean Delvare <jdelvare@...e.com>,
        linux-hwmon@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the
 ACPI global lock

On 3/29/22 15:11, Eugene Shalygin wrote:
> On Tue, 29 Mar 2022 at 23:23, Guenter Roeck <linux@...ck-us.net> wrote:
> 
>>> +/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
>>> +#define ACPI_GLOBAL_LOCK_PSEUDO_PATH ":GLOBAL_LOCK"
>>> +
>>
>> That needs to be documented.
> 
> Do you mean a note in the /Documentation/..../...rst or adding details
> here? There is an additional bit of information on this identifier
> below, in the ec_board_info struct declaration.
> 
My understanding was that the user would/could request its use via
the module parameter, so it needs to be documented in the rst file.

>> There is some type confusion in the above lock functions. Some return
>> ACPI error codes, some return Linux error codes. Please make return
>> values consistent.
>>
>> Also, why use mutex_trylock() instead of mutex_lock() ? This is
>> unusual since it will result in errors if more than one user
>> tries to access the data (eg multiple processes reading sysfs
>> attributes at the same time), and thus warrants a detailed
>> explanation.
> OK.
> 
>>> +     struct lock_data lock_data;
>>> +     /* number of board EC sensors */
>>> +     u8 nr_sensors;
>>
>> Ok, I must admit I am more than a bit lost. In patch 1/4
>> you removed this variable (and argued that removing it was
>> for "deduplication"), only to re-introduce it here.
>> Sorry, I don't follow the logic.
> 
> Sorry for that. This is my mistake which I tried to warn you about in
> my first reply to the email with this patch.
> 
>>> +     if (!mutex_path || !strlen(mutex_path)) {
>>
>> When would mutex_path be NULL ?
> When it is set to NULL in the board definition struct ec_board_info.
> 

Are there any such board definitions ? I don't recall seeing any.

Thanks,
Guenter

>>> +             if (ACPI_FAILURE(status)) {
>>> +                     dev_err(dev,
>>> +                             "Failed to get hardware access guard AML mutex"
>>> +                             "'%s': error %d",
>>
>> Please no string splits. And the negative impact can be seen here:
>> No space between "mutex" and "'%s'".
> 
> Yes, of course.
> 
>>>                dev_warn(dev,
>>> -                     "Concurrent access to the ACPI EC detected.\nRace condition possible.");
>>> +                     "Concurrent access to the ACPI EC detected.\n"
>>> +                     "Race condition possible.");
>>
>> Why this change, and how is it related to this patch ?
> Same as above, will be corrected.
> 
> Thank you,
> Eugene