Re: [PATCH] mpi3mr: Fix potential deadlock in mpi3mr_fault_uevent_emit

Re: [PATCH] mpi3mr: Fix potential deadlock in mpi3mr_fault_uevent_emit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete deadlock fix: kobject_uevent_env() unconditionally performs GFP_KERNEL allocations
--

--- Patch [1]: [PATCH] mpi3mr: Fix potential deadlock in mpi3mr_fault_uevent_emit ---
commit 7bbe5234e6fc72077b0934f22beb787676fbb5b2
Author: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

mpi3mr: Fix potential deadlock in mpi3mr_fault_uevent_emit
    
Change GFP_KERNEL to GFP_NOIO in mpi3mr_fault_uevent_emit() to avoid
a potential deadlock during memory reclaim when the controller is in
a fault state and cannot process I/O.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> index 31b19ed1528e5..e772874ea77ce 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> @@ -1289,7 +1289,7 @@ static void mpi3mr_fault_uevent_emit(struct mpi3mr_ioc *mrioc)
>  	struct kobj_uevent_env *env;
>  	int ret;
>  
> -	env = kzalloc_obj(*env);
> +	env = kzalloc_obj(*env, GFP_NOIO);
>  	if (!env)
>  		return;
>  

[Severity: High]
This is a pre-existing issue, but does this patch fully prevent the deadlock
it claims to fix?

While the local environment allocation is updated to use GFP_NOIO, this
function later calls kobject_uevent_env():

drivers/scsi/mpi3mr/mpi3mr_fw.c:mpi3mr_fault_uevent_emit() {
    ...
    env = kzalloc_obj(*env, GFP_NOIO);
    ...
    kobject_uevent_env(&mrioc->shost->shost_gendev.kobj,
                       KOBJ_CHANGE, env->envp);
}

The kobject_uevent_env() API unconditionally performs multiple memory
allocations using GFP_KERNEL. For example, in lib/kobject_uevent.c:

lib/kobject_uevent.c:kobject_uevent_env() {
    ...
    env = kzalloc_obj(struct kobj_uevent_env);
    ...
    devpath = kobject_get_path(kobj, GFP_KERNEL);
    ...
}

And also when allocating the netlink broadcast socket buffer:

lib/kobject_uevent.c:alloc_uevent_skb() {
    ...
    skb = alloc_skb(len + env->buflen, GFP_KERNEL);
    ...
}

Since these nested calls still rely on GFP_KERNEL, can the deadlock still
occur during memory reclaim when the system is under memory pressure and the
storage controller is in a fault state?

Would it be better to use memalloc_noio_save() and memalloc_noio_restore()
to properly apply NOIO semantics to all nested allocations during the fault
uevent emission?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724164630.924288-1-chandrakanth.patil@broadcom.com?part=1

[PATCH] mpi3mr: Fix potential deadlock in mpi3mr_fault_uevent_emit

[Chandrakanth Patil]

Change GFP_KERNEL to GFP_NOIO in mpi3mr_fault_uevent_emit() to avoid
a potential deadlock during memory reclaim when the controller is in
a fault state and cannot process I/O.

Fixes: ec54b348f274 ("scsi: mpi3mr: Record and report controller firmware faults")
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
 drivers/scsi/mpi3mr/mpi3mr_fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 31b19ed1528e..e772874ea77c 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -1289,7 +1289,7 @@ static void mpi3mr_fault_uevent_emit(struct mpi3mr_ioc *mrioc)
 	struct kobj_uevent_env *env;
 	int ret;
 
-	env = kzalloc_obj(*env);
+	env = kzalloc_obj(*env, GFP_NOIO);
 	if (!env)
 		return;
 
-- 
2.52.0

[PATCH v2] mpi3mr: Fix potential deadlock in mpi3mr_fault_uevent_emit

[Chandrakanth Patil]

mpi3mr_fault_uevent_emit() runs from the fault watchdog and reset
paths, where host I/O may already be blocked. GFP_KERNEL allocations
here, both the local kzalloc_obj() and the ones inside
kobject_uevent_env() itself, can trigger reclaim that waits on that
blocked I/O and deadlock.

Use memalloc_noio_save()/restore() to cover the whole call instead of
just the local allocation.

Fixes: ec54b348f274 ("scsi: mpi3mr: Record and report controller firmware faults")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260724164630.924288-1-chandrakanth.patil%40broadcom.com
Signed-off-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
---
V1 -> V2: Wrap kobject_uevent_env() in memalloc_noio_save()/restore()
    instead of only changing the local kzalloc_obj() allocation to
    GFP_NOIO, since kobject_uevent_env() performs unconditional
    GFP_KERNEL allocations internally too. (Sashiko)

 drivers/scsi/mpi3mr/mpi3mr_fw.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c b/drivers/scsi/mpi3mr/mpi3mr_fw.c
index 31b19ed1528e..681868716ebd 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
@@ -9,6 +9,7 @@
 
 #include "mpi3mr.h"
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/sched/mm.h>
 
 static int
 mpi3mr_issue_reset(struct mpi3mr_ioc *mrioc, u16 reset_type, u16 reset_reason);
@@ -1287,11 +1288,14 @@ static void mpi3mr_alloc_ioctl_dma_memory(struct mpi3mr_ioc *mrioc)
 static void mpi3mr_fault_uevent_emit(struct mpi3mr_ioc *mrioc)
 {
 	struct kobj_uevent_env *env;
+	unsigned int noio_flag;
 	int ret;
 
+	noio_flag = memalloc_noio_save();
+
 	env = kzalloc_obj(*env);
 	if (!env)
-		return;
+		goto out_restore;
 
 	ret = add_uevent_var(env, "DRIVER=%s", mrioc->driver_name);
 	if (ret)
@@ -1326,7 +1330,8 @@ static void mpi3mr_fault_uevent_emit(struct mpi3mr_ioc *mrioc)
 
 out_free:
 	kfree(env);
-
+out_restore:
+	memalloc_noio_restore(noio_flag);
 }
 
 /**
-- 
2.52.0