| 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: <a0c294e1-f76a-6382-ee0f-f1d75ac9d781@quicinc.com>
Date: Mon, 23 Oct 2023 18:11:18 +0530
From: Mukesh Ojha <quic_mojha@...cinc.com>
To: Luis Chamberlain <mcgrof@...nel.org>
CC: <russell.h.weight@...el.com>, <gregkh@...uxfoundation.org>,
<rafael@...nel.org>, <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Christophe Leroy <christophe.leroy@...roup.eu>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v3] firmware_loader: Abort new fw load request once
firmware core knows about reboot
On 10/20/2023 12:36 AM, Luis Chamberlain wrote:
> On Thu, Oct 05, 2023 at 10:37:00AM +0530, Mukesh Ojha wrote:
>> There could be following scenario where there is a ongoing reboot
>> is going from processA which tries to call all the reboot notifier
>> callback and one of them is firmware reboot call which tries to
>> abort all the ongoing firmware userspace request under fw_lock but
>> there could be another processB which tries to do request firmware,
>> which came just after abort done from ProcessA and ask for userspace
>> to load the firmware and this can stop the ongoing reboot ProcessA
>> to stall for next 60s(default timeout) which may not be expected
>> behaviour everyone like to see, instead we should abort any firmware
>> load request which came once firmware knows about the reboot through
>> notification.
>>
>> ProcessA ProcessB
>>
>> kernel_restart_prepare
>> blocking_notifier_call_chain
>> fw_shutdown_notify
>> kill_pending_fw_fallback_reqs
>> __fw_load_abort
>> fw_state_aborted request_firmware
>> __fw_state_set firmware_fallback_sysfs
>> ... fw_load_from_user_helper
>> .. ...
>> . ..
>> usermodehelper_read_trylock
>> fw_load_sysfs_fallback
>> fw_sysfs_wait_timeout
>> usermodehelper_disable
>> __usermodehelper_disable
>> down_write()
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
>> ---
>> Changes from v2: https://lore.kernel.org/lkml/1696431327-7369-1-git-send-email-quic_mojha@quicinc.com/
>> - Renamed the flag to fw_abort_load.
>>
>> Changes from v1: https://lore.kernel.org/lkml/1694773288-15755-1-git-send-email-quic_mojha@quicinc.com/
>> - Renamed the flag to fw_load_abort.
>> - Kept the flag under fw_lock.
>> - Repharsed commit text.
>>
>> drivers/base/firmware_loader/fallback.c | 6 +++++-
>> drivers/base/firmware_loader/firmware.h | 1 +
>> drivers/base/firmware_loader/main.c | 1 +
>> 3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
>> index bf68e3947814..a162020e98f2 100644
>> --- a/drivers/base/firmware_loader/fallback.c
>> +++ b/drivers/base/firmware_loader/fallback.c
>> @@ -57,6 +57,10 @@ void kill_pending_fw_fallback_reqs(bool only_kill_custom)
>
> This routine uses a bool for only_kill_custom for when the events we
> should kill ar ecustom or not. Piggy backing on it to assume that the
> negative of value represents a shutdown is abusing the semantics
> and muddies the waters. So to avoid that, just extend the arguments
> to kill_pending_fw_fallback_reqs() for a new bool shutdown, that allows
> the code to be clearer and the intent is kept clear.
Let me understand what does 'only_kill_custom' do,
1. It gets called from reboot notifier where it's default value is
false. So, the intention here is to kill all the ongoing request along
with !buf->need_uevent ?
commit c4b768934be613fb882e4e4090946218d76c8e1b
Author: Luis R. Rodriguez <mcgrof@...nel.org>
Date: Tue May 2 01:31:03 2017 -0700
firmware: share fw fallback killing on reboot/suspend
We kill pending fallback requests on suspend and reboot,
the only difference is that on suspend we only kill custom
fallback requests. Provide a wrapper that lets us customize
the request with a flag.
This also lets us simplify the #ifdef'ery over the calls.
Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
2. The second call is from fw_pm_notify() which is only
interested in aborting non-uevent calls.
And in this patch since, we are calling it from reboot which is the
first case, so piggybacking on the 'only_kill_custom' would be fine for
this patch. However, if you think we should rename this
'only_kill_custom' to something like its inverse 'kill_all' and reverse
the below check to be more meaningful now ?
if (kill_all || !fw_priv->need_uevent)
>
>> if (!fw_priv->need_uevent || !only_kill_custom)
>> __fw_load_abort(fw_priv);
>> }
>> +
>> + if (!only_kill_custom)
>> + fw_abort_load = true;
>> +
>> mutex_unlock(&fw_lock);
>> }
>>
>> @@ -86,7 +90,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
>> }
>>
>> mutex_lock(&fw_lock);
>> - if (fw_state_is_aborted(fw_priv)) {
>> + if (fw_abort_load || fw_state_is_aborted(fw_priv)) {
>
> However, do we really need this ? Could we just use:
>
> if (system_state == SYSTEM_HALT ||
> system_state == SYSTEM_POWER_OFF ||
> system_state == SYSTEM_RESTART ||
> fw_state_is_aborted(fw_priv))
>
> ?
There is slight window where system_state is not yet set and other
reboot notifiers after firmware one are running with ongoing
request_firmware().
-Mukesh
>
> Luis
Powered by blists - more mailing lists