mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling
@ 2025-05-28  8:49 Sudeep Holla
  2025-05-28  8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28  8:49 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
	Jérôme Forissier

This patch series improves the arm_ffa notification callback handling by
addressing safety and memory issues. It replaces a mutex with an rwlock
to prevent sleeping in atomic context, resolving kernel warnings. Memory
allocation is moved outside the lock to support this transition safely.
Additionally, a memory leak in the notifier unregistration path is fixed
by properly freeing the callback node.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
Sudeep Holla (3):
      firmware: arm_ffa: Fix memory leak by freeing notifier callback node
      firmware: arm_ffa: Move memory allocation outside the mutex locking
      firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context

 drivers/firmware/arm_ffa/driver.c | 71 ++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 35 deletions(-)
---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250528-ffa_notif_fix-f90903a2ed0b
-- 
Regards,
Sudeep


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node
  2025-05-28  8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
@ 2025-05-28  8:49 ` Sudeep Holla
  2025-05-28 15:31   ` Jens Wiklander
  2025-05-28  8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28  8:49 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
	Jérôme Forissier

Commit e0573444edbf ("firmware: arm_ffa: Add interfaces to request
notification callbacks") adds support for notifier callbacks by allocating
and inserting a callback node into a hashtable during registration of
notifiers. However, during unregistration, the code only removes the
node from the hashtable without freeing the associated memory, resulting
in a memory leak.

Resolve the memory leak issue by ensuring the allocated notifier callback
node is properly freed after it is removed from the hashtable entry.

Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index fe55613a8ea993378474671d3c5756309da34fbc..6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1284,6 +1284,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
 		hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
 	} else {
 		hash_del(&cb_info->hnode);
+		kfree(cb_info);
 	}
 
 	return 0;

-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking
  2025-05-28  8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
  2025-05-28  8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
@ 2025-05-28  8:49 ` Sudeep Holla
  2025-05-28 15:33   ` Jens Wiklander
  2025-05-28  8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
  2025-06-09 10:31 ` [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
  3 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28  8:49 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
	Jérôme Forissier

The notifier callback node allocation is currently done while holding
the notify_lock mutex. While this is safe even if memory allocation may
sleep, we need to move the allocation outside the locked region in
preparation to move from using muxtes to rwlocks.

Move the memory allocation to avoid potential sleeping in atomic context
once the locks are moved from mutex to rwlocks.

Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 48 +++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de..44eecb786e67b205161e2d48602e1f1b53533360 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1250,13 +1250,12 @@ notifier_hnode_get_by_type(u16 notify_id, enum notify_type type)
 	return NULL;
 }
 
-static int
-update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
-		   void *cb_data, bool is_registration, bool is_framework)
+static int update_notifier_cb(struct ffa_device *dev, int notify_id,
+			      struct notifier_cb_info *cb, bool is_framework)
 {
 	struct notifier_cb_info *cb_info = NULL;
 	enum notify_type type = ffa_notify_type_get(dev->vm_id);
-	bool cb_found;
+	bool cb_found, is_registration = !!cb;
 
 	if (is_framework)
 		cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, dev->vm_id,
@@ -1270,18 +1269,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
 		return -EINVAL;
 
 	if (is_registration) {
-		cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
-		if (!cb_info)
-			return -ENOMEM;
-
-		cb_info->dev = dev;
-		cb_info->cb_data = cb_data;
-		if (is_framework)
-			cb_info->fwk_cb = cb;
-		else
-			cb_info->cb = cb;
-
-		hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
+		hash_add(drv_info->notifier_hash, &cb->hnode, notify_id);
 	} else {
 		hash_del(&cb_info->hnode);
 		kfree(cb_info);
@@ -1303,8 +1291,7 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
 
 	mutex_lock(&drv_info->notify_lock);
 
-	rc = update_notifier_cb(dev, notify_id, NULL, NULL, false,
-				is_framework);
+	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);
@@ -1335,6 +1322,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
 {
 	int rc;
 	u32 flags = 0;
+	struct notifier_cb_info *cb_info = NULL;
 
 	if (ffa_notifications_disabled())
 		return -EOPNOTSUPP;
@@ -1342,6 +1330,17 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
 	if (notify_id >= FFA_MAX_NOTIFICATIONS)
 		return -EINVAL;
 
+	cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
+	if (!cb_info)
+		return -ENOMEM;
+
+	cb_info->dev = dev;
+	cb_info->cb_data = cb_data;
+	if (is_framework)
+		cb_info->fwk_cb = cb;
+	else
+		cb_info->cb = cb;
+
 	mutex_lock(&drv_info->notify_lock);
 
 	if (!is_framework) {
@@ -1349,21 +1348,22 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
 			flags = PER_VCPU_NOTIFICATION_FLAG;
 
 		rc = ffa_notification_bind(dev->vm_id, BIT(notify_id), flags);
-		if (rc) {
-			mutex_unlock(&drv_info->notify_lock);
-			return rc;
-		}
+		if (rc)
+			goto out_unlock_free;
 	}
 
-	rc = update_notifier_cb(dev, notify_id, cb, cb_data, true,
-				is_framework);
+	rc = update_notifier_cb(dev, notify_id, cb_info, is_framework);
 	if (rc) {
 		pr_err("Failed to register callback for %d - %d\n",
 		       notify_id, rc);
 		if (!is_framework)
 			ffa_notification_unbind(dev->vm_id, BIT(notify_id));
 	}
+
+out_unlock_free:
 	mutex_unlock(&drv_info->notify_lock);
+	if (rc)
+		kfree(cb_info);
 
 	return rc;
 }

-- 
2.34.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context
  2025-05-28  8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
  2025-05-28  8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
  2025-05-28  8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
@ 2025-05-28  8:49 ` Sudeep Holla
  2025-05-28 15:34   ` Jens Wiklander
  2025-06-09 10:31 ` [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
  3 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28  8:49 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
	Jérôme Forissier

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@linaro.org>
Reported-by: Jérôme Forissier <jerome.forissier@linaro.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@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node
  2025-05-28  8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
@ 2025-05-28 15:31   ` Jens Wiklander
  0 siblings, 0 replies; 8+ messages in thread
From: Jens Wiklander @ 2025-05-28 15:31 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-arm-kernel, linux-kernel, Jérôme Forissier

On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Commit e0573444edbf ("firmware: arm_ffa: Add interfaces to request
> notification callbacks") adds support for notifier callbacks by allocating
> and inserting a callback node into a hashtable during registration of
> notifiers. However, during unregistration, the code only removes the
> node from the hashtable without freeing the associated memory, resulting
> in a memory leak.
>
> Resolve the memory leak issue by ensuring the allocated notifier callback
> node is properly freed after it is removed from the hashtable entry.
>
> Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Cheers,
Jens

>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index fe55613a8ea993378474671d3c5756309da34fbc..6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1284,6 +1284,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
>                 hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
>         } else {
>                 hash_del(&cb_info->hnode);
> +               kfree(cb_info);
>         }
>
>         return 0;
>
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking
  2025-05-28  8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
@ 2025-05-28 15:33   ` Jens Wiklander
  0 siblings, 0 replies; 8+ messages in thread
From: Jens Wiklander @ 2025-05-28 15:33 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-arm-kernel, linux-kernel, Jérôme Forissier

On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The notifier callback node allocation is currently done while holding
> the notify_lock mutex. While this is safe even if memory allocation may
> sleep, we need to move the allocation outside the locked region in
> preparation to move from using muxtes to rwlocks.
>
> Move the memory allocation to avoid potential sleeping in atomic context
> once the locks are moved from mutex to rwlocks.
>
> Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 48 +++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Cheers,
Jens

>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de..44eecb786e67b205161e2d48602e1f1b53533360 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1250,13 +1250,12 @@ notifier_hnode_get_by_type(u16 notify_id, enum notify_type type)
>         return NULL;
>  }
>
> -static int
> -update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
> -                  void *cb_data, bool is_registration, bool is_framework)
> +static int update_notifier_cb(struct ffa_device *dev, int notify_id,
> +                             struct notifier_cb_info *cb, bool is_framework)
>  {
>         struct notifier_cb_info *cb_info = NULL;
>         enum notify_type type = ffa_notify_type_get(dev->vm_id);
> -       bool cb_found;
> +       bool cb_found, is_registration = !!cb;
>
>         if (is_framework)
>                 cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, dev->vm_id,
> @@ -1270,18 +1269,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
>                 return -EINVAL;
>
>         if (is_registration) {
> -               cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
> -               if (!cb_info)
> -                       return -ENOMEM;
> -
> -               cb_info->dev = dev;
> -               cb_info->cb_data = cb_data;
> -               if (is_framework)
> -                       cb_info->fwk_cb = cb;
> -               else
> -                       cb_info->cb = cb;
> -
> -               hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
> +               hash_add(drv_info->notifier_hash, &cb->hnode, notify_id);
>         } else {
>                 hash_del(&cb_info->hnode);
>                 kfree(cb_info);
> @@ -1303,8 +1291,7 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
>
>         mutex_lock(&drv_info->notify_lock);
>
> -       rc = update_notifier_cb(dev, notify_id, NULL, NULL, false,
> -                               is_framework);
> +       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);
> @@ -1335,6 +1322,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
>  {
>         int rc;
>         u32 flags = 0;
> +       struct notifier_cb_info *cb_info = NULL;
>
>         if (ffa_notifications_disabled())
>                 return -EOPNOTSUPP;
> @@ -1342,6 +1330,17 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
>         if (notify_id >= FFA_MAX_NOTIFICATIONS)
>                 return -EINVAL;
>
> +       cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
> +       if (!cb_info)
> +               return -ENOMEM;
> +
> +       cb_info->dev = dev;
> +       cb_info->cb_data = cb_data;
> +       if (is_framework)
> +               cb_info->fwk_cb = cb;
> +       else
> +               cb_info->cb = cb;
> +
>         mutex_lock(&drv_info->notify_lock);
>
>         if (!is_framework) {
> @@ -1349,21 +1348,22 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
>                         flags = PER_VCPU_NOTIFICATION_FLAG;
>
>                 rc = ffa_notification_bind(dev->vm_id, BIT(notify_id), flags);
> -               if (rc) {
> -                       mutex_unlock(&drv_info->notify_lock);
> -                       return rc;
> -               }
> +               if (rc)
> +                       goto out_unlock_free;
>         }
>
> -       rc = update_notifier_cb(dev, notify_id, cb, cb_data, true,
> -                               is_framework);
> +       rc = update_notifier_cb(dev, notify_id, cb_info, is_framework);
>         if (rc) {
>                 pr_err("Failed to register callback for %d - %d\n",
>                        notify_id, rc);
>                 if (!is_framework)
>                         ffa_notification_unbind(dev->vm_id, BIT(notify_id));
>         }
> +
> +out_unlock_free:
>         mutex_unlock(&drv_info->notify_lock);
> +       if (rc)
> +               kfree(cb_info);
>
>         return rc;
>  }
>
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context
  2025-05-28  8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
@ 2025-05-28 15:34   ` Jens Wiklander
  0 siblings, 0 replies; 8+ messages in thread
From: Jens Wiklander @ 2025-05-28 15:34 UTC (permalink / raw)
  To: Sudeep Holla; +Cc: linux-arm-kernel, linux-kernel, Jérôme Forissier

On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@arm.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@linaro.org>
> Reported-by: Jérôme Forissier <jerome.forissier@linaro.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@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by: Jens Wiklander <jens.wiklander@linaro.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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling
  2025-05-28  8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
                   ` (2 preceding siblings ...)
  2025-05-28  8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
@ 2025-06-09 10:31 ` Sudeep Holla
  3 siblings, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2025-06-09 10:31 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Jens Wiklander,
	Jérôme Forissier, Sudeep Holla

On Wed, 28 May 2025 09:49:40 +0100, Sudeep Holla wrote:
> This patch series improves the arm_ffa notification callback handling by
> addressing safety and memory issues. It replaces a mutex with an rwlock
> to prevent sleeping in atomic context, resolving kernel warnings. Memory
> allocation is moved outside the lock to support this transition safely.
> Additionally, a memory leak in the notifier unregistration path is fixed
> by properly freeing the callback node.
> 
> [...]

Applied to sudeep.holla/linux (for-next/ffa/fixes), thanks!

[1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node
      https://git.kernel.org/sudeep.holla/c/a833d31ad867
[2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking
      https://git.kernel.org/sudeep.holla/c/27e850c88df0
[3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context
      https://git.kernel.org/sudeep.holla/c/9ca7a421229b

--
Regards,
Sudeep


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-06-09 10:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-05-28  8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
2025-05-28  8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
2025-05-28 15:31   ` Jens Wiklander
2025-05-28  8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
2025-05-28 15:33   ` Jens Wiklander
2025-05-28  8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
2025-05-28 15:34   ` Jens Wiklander
2025-06-09 10:31 ` [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®