| 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: <CAHUa44HRsYUV8U-jBYvhsPoLNizXamxYK+Th+O4ZJ2VpRQ9A7A@mail.gmail.com>
Date: Wed, 28 May 2025 17:34:40 +0200
From: Jens Wiklander <jens.wiklander@...aro.org>
To: Sudeep Holla <sudeep.holla@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Jérôme Forissier <jerome.forissier@...aro.org>
Subject: Re: [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid
sleep in atomic context
On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@....com> wrote:
>
> The current use of a mutex to protect the notifier hashtable accesses
> can lead to issues in the atomic context. It results in the below
> kernel warnings:
>
> | BUG: sleeping function called from invalid context at kernel/locking/mutex.c:258
> | in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 9, name: kworker/0:0
> | preempt_count: 1, expected: 0
> | RCU nest depth: 0, expected: 0
> | CPU: 0 UID: 0 PID: 9 Comm: kworker/0:0 Not tainted 6.14.0 #4
> | Workqueue: ffa_pcpu_irq_notification notif_pcpu_irq_work_fn
> | Call trace:
> | show_stack+0x18/0x24 (C)
> | dump_stack_lvl+0x78/0x90
> | dump_stack+0x18/0x24
> | __might_resched+0x114/0x170
> | __might_sleep+0x48/0x98
> | mutex_lock+0x24/0x80
> | handle_notif_callbacks+0x54/0xe0
> | notif_get_and_handle+0x40/0x88
> | generic_exec_single+0x80/0xc0
> | smp_call_function_single+0xfc/0x1a0
> | notif_pcpu_irq_work_fn+0x2c/0x38
> | process_one_work+0x14c/0x2b4
> | worker_thread+0x2e4/0x3e0
> | kthread+0x13c/0x210
> | ret_from_fork+0x10/0x20
>
> To address this, replace the mutex with an rwlock to protect the notifier
> hashtable accesses. This ensures that read-side locking does not sleep and
> multiple readers can acquire the lock concurrently, avoiding unnecessary
> contention and potential deadlocks. Writer access remains exclusive,
> preserving correctness.
>
> This change resolves warnings from lockdep about potential sleep in
> atomic context.
>
> Cc: Jens Wiklander <jens.wiklander@...aro.org>
> Reported-by: Jérôme Forissier <jerome.forissier@...aro.org>
> Closes: https://github.com/OP-TEE/optee_os/issues/7394
> Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
> drivers/firmware/arm_ffa/driver.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Jens Wiklander <jens.wiklander@...aro.org>
Tested-by: Jens Wiklander <jens.wiklander@...aro.org>
Cheers,
Jens
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 44eecb786e67b205161e2d48602e1f1b53533360..37eb2e6c2f9f4d30831b7bf6ce5142a39a19f50c 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -110,7 +110,7 @@ struct ffa_drv_info {
> struct work_struct sched_recv_irq_work;
> struct xarray partition_info;
> DECLARE_HASHTABLE(notifier_hash, ilog2(FFA_MAX_NOTIFICATIONS));
> - struct mutex notify_lock; /* lock to protect notifier hashtable */
> + rwlock_t notify_lock; /* lock to protect notifier hashtable */
> };
>
> static struct ffa_drv_info *drv_info;
> @@ -1289,19 +1289,19 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
> if (notify_id >= FFA_MAX_NOTIFICATIONS)
> return -EINVAL;
>
> - mutex_lock(&drv_info->notify_lock);
> + write_lock(&drv_info->notify_lock);
>
> rc = update_notifier_cb(dev, notify_id, NULL, is_framework);
> if (rc) {
> pr_err("Could not unregister notification callback\n");
> - mutex_unlock(&drv_info->notify_lock);
> + write_unlock(&drv_info->notify_lock);
> return rc;
> }
>
> if (!is_framework)
> rc = ffa_notification_unbind(dev->vm_id, BIT(notify_id));
>
> - mutex_unlock(&drv_info->notify_lock);
> + write_unlock(&drv_info->notify_lock);
>
> return rc;
> }
> @@ -1341,7 +1341,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> else
> cb_info->cb = cb;
>
> - mutex_lock(&drv_info->notify_lock);
> + write_lock(&drv_info->notify_lock);
>
> if (!is_framework) {
> if (is_per_vcpu)
> @@ -1361,7 +1361,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> }
>
> out_unlock_free:
> - mutex_unlock(&drv_info->notify_lock);
> + write_unlock(&drv_info->notify_lock);
> if (rc)
> kfree(cb_info);
>
> @@ -1407,9 +1407,9 @@ static void handle_notif_callbacks(u64 bitmap, enum notify_type type)
> if (!(bitmap & 1))
> continue;
>
> - mutex_lock(&drv_info->notify_lock);
> + read_lock(&drv_info->notify_lock);
> cb_info = notifier_hnode_get_by_type(notify_id, type);
> - mutex_unlock(&drv_info->notify_lock);
> + read_unlock(&drv_info->notify_lock);
>
> if (cb_info && cb_info->cb)
> cb_info->cb(notify_id, cb_info->cb_data);
> @@ -1447,9 +1447,9 @@ static void handle_fwk_notif_callbacks(u32 bitmap)
>
> ffa_rx_release();
>
> - mutex_lock(&drv_info->notify_lock);
> + read_lock(&drv_info->notify_lock);
> cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, target, &uuid);
> - mutex_unlock(&drv_info->notify_lock);
> + read_unlock(&drv_info->notify_lock);
>
> if (cb_info && cb_info->fwk_cb)
> cb_info->fwk_cb(notify_id, cb_info->cb_data, buf);
> @@ -1974,7 +1974,7 @@ static void ffa_notifications_setup(void)
> goto cleanup;
>
> hash_init(drv_info->notifier_hash);
> - mutex_init(&drv_info->notify_lock);
> + rwlock_init(&drv_info->notify_lock);
>
> drv_info->notif_enabled = true;
> return;
>
> --
> 2.34.1
>
Powered by blists - more mailing lists