| 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
| ||
|
Message-ID: <f6ac14a8-6403-029e-6179-f8ef0a7d5457@gmx.de>
Date: Tue, 27 Sep 2022 17:44:18 +0200
From: Armin Wolf <W_Armin@....de>
To: Hans de Goede <hdegoede@...hat.com>,
"Rafael J. Wysocki" <rafael@...nel.org>
Cc: Mark Gross <markgross@...nel.org>, Len Brown <lenb@...nel.org>,
Henrique de Moraes Holschuh <hmh@....eng.br>,
Matan Ziv-Av <matan@...alib.org>,
Corentin Chary <corentin.chary@...il.com>,
Jeremy Soller <jeremy@...tem76.com>, productdev@...tem76.com,
Platform Driver <platform-driver-x86@...r.kernel.org>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single
error
Am 27.09.22 um 16:29 schrieb Hans de Goede:
> Hi,
>
> On 9/19/22 22:35, Armin Wolf wrote:
>> Am 19.09.22 um 21:12 schrieb Armin Wolf:
>>
>>> Am 19.09.22 um 18:27 schrieb Rafael J. Wysocki:
>>>
>>>> On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@...hat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 9/12/22 13:53, Armin Wolf wrote:
>>>>>> Currently, battery hooks are being unloaded if they return
>>>>>> an error when adding a single battery.
>>>>>> This however also causes the removal of successfully added
>>>>>> hooks if they return -ENODEV for a single unsupported
>>>>>> battery.
>>>>>>
>>>>>> Do not unload battery hooks in such cases since the hook
>>>>>> handles any cleanup actions.
>>>>>>
>>>>>> Signed-off-by: Armin Wolf <W_Armin@....de>
>>>>> Maybe instead of removing all error checking, allow -ENODEV
>>>>> and behave as before when the error is not -ENODEV ?
>>>>>
>>>>> Otherwise we should probably make the add / remove callbacks
>>>>> void to indicate that any errors are ignored.
>>>>>
>>>>> Rafael, do you have any opinion on this?
>>>> IMV this is not a completely safe change, because things may simply
>>>> not work in the cases in which an error is returned.
>>>>
>>>> It would be somewhat better to use a special error code to indicate
>>>> "no support" (eg. -ENOTSUPP) and ignore that one only.
>>> I would favor -ENODEV then, since it is already used by quiet a few drivers
>>> to indicate a unsupported battery.
>>>
>>> Armin Wolf
>>>
>> While checking all instances where the battery hook mechanism is currently used,
>> i found out that all but a single battery hook return -ENODEV for all errors they
>> encounter, the exception being the huawei-wmi driver.
> Right, so this means that using -ENODEV to not automatically unload the
> extension on error will result in a behavior change for those drivers,
> with possibly unwanted side-effects.
>
> As such I believe that using -ENOTSUP for the case where the extension
> does not work for 1 battery but should still be used for the other
> batter{y|ies} would be better as this preserves the existing behavior
> for existing drivers.
>
>> I do not know the reason for this, but i fear unloading the extension on for
>> example -ENOTSUP will result in similar behavior by hooks wanting to avoid being
>> unloaded on harmless errors.
> I am not sure what you are trying to say here. The whole idea is
> to add new behavior for -ENOTSUP to allow drivers to opt out of
> getting their extension unregistered when they return this.
>
> Although I wonder why not just have extensions return 0 when
> they don't want to register any sysfs attr and that not considered
> an error. If it is not considered an error the hook can just
> return 0, which would not require any ACPI battery code changes
> at all. So maybe just returning 0 is the easiest (which is
> also often the best) answer here?
I agree, i will send v2 soon.
Armin Wolf
>> However, i agree that when ignoring all errors, battery extensions which provide
>> similar attributes may currently delete each others attributes.
> AFAIK there are no cases where more then 1 extension driver gets loaded,
> since all the extension drivers are tied to a specific vendor's interfaces
> so we won't e.g. see the thinkpad_acpi driver load on the same laptop as
> where toshiba_acpi also loads.
>
> IOW I think you are trying to solve a problem which does not exist here.
>
> Regards,
>
> Hans
>
>
>
>
>> Any idea on how to solve this?
>>
>> Armin Wolf
>>
>>>>>> ---
>>>>>> drivers/acpi/battery.c | 24 +++---------------------
>>>>>> 1 file changed, 3 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>>>>> index 306513fec1e1..e59c261c7c59 100644
>>>>>> --- a/drivers/acpi/battery.c
>>>>>> +++ b/drivers/acpi/battery.c
>>>>>> @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>>>>>> * its attributes.
>>>>>> */
>>>>>> list_for_each_entry(battery, &acpi_battery_list, list) {
>>>>>> - if (hook->add_battery(battery->bat)) {
>>>>>> - /*
>>>>>> - * If a add-battery returns non-zero,
>>>>>> - * the registration of the extension has failed,
>>>>>> - * and we will not add it to the list of loaded
>>>>>> - * hooks.
>>>>>> - */
>>>>>> - pr_err("extension failed to load: %s", hook->name);
>>>>>> - __battery_hook_unregister(hook, 0);
>>>>>> - goto end;
>>>>>> - }
>>>>>> + hook->add_battery(battery->bat);
>>>>>> }
>>>>>> pr_info("new extension: %s\n", hook->name);
>>>>>> -end:
>>>>>> +
>>>>>> mutex_unlock(&hook_mutex);
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(battery_hook_register);
>>>>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>>>>>> * during the battery module initialization.
>>>>>> */
>>>>>> list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
>>>>>> - if (hook_node->add_battery(battery->bat)) {
>>>>>> - /*
>>>>>> - * The notification of the extensions has failed, to
>>>>>> - * prevent further errors we will unload the extension.
>>>>>> - */
>>>>>> - pr_err("error in extension, unloading: %s",
>>>>>> - hook_node->name);
>>>>>> - __battery_hook_unregister(hook_node, 0);
>>>>>> - }
>>>>>> + hook_node->add_battery(battery->bat);
>>>>>> }
>>>>>> mutex_unlock(&hook_mutex);
>>>>>> }
>>>>>> --
>>>>>> 2.30.2
>>>>>>
Powered by blists - more mailing lists