[PATCH] RDMA/irdma: publish APBVT entries after hardware setup

[PATCH] RDMA/irdma: publish APBVT entries after hardware setup

[Jaeyeong Lee]

irdma_add_apbvt() inserts a new entry into apbvt_hash_tbl before
sending the CQP command that creates the hardware entry. If the command
fails, the entry is freed without being removed from the hash table. A
later lookup of the same port traverses the freed entry.

Removing the entry on failure is not sufficient because another thread
can find it and increment use_cnt while the CQP command is in progress.

Serialize APBVT additions and publish new entries only after the hardware
command succeeds. Since the allocation no longer runs under apbvt_lock,
use GFP_KERNEL.

The issue was found by static analysis and confirmed with a KASAN
reproducer that forces the CQP failure and retries the same port.

BUG: KASAN: slab-use-after-free in irdma_add_apbvt+0xa0/0x280 [irdma]
Read of size 2 at addr ffff8880092d10d4 by task insmod/72

Call Trace:
 irdma_add_apbvt+0xa0/0x280 [irdma]
 init_module+0x232/0xff0 [irdma_apbvt_repro]
 do_one_initcall+0x13d/0x340

Allocated by task 72:
 irdma_add_apbvt+0xda/0x280 [irdma]
 init_module+0x210/0xff0 [irdma_apbvt_repro]

Freed by task 72:
 kfree+0x155/0x470
 irdma_add_apbvt+0x1e9/0x280 [irdma]
 init_module+0x210/0xff0 [irdma_apbvt_repro]

Fixes: 44d9e52977a1 ("RDMA/irdma: Implement device initialization definitions")
Cc: stable@vger.kernel.org
Signed-off-by: Jaeyeong Lee <lee@jaeyeong.cc>
---
 drivers/infiniband/hw/irdma/cm.c |  1 +
 drivers/infiniband/hw/irdma/cm.h |  3 ++-
 drivers/infiniband/hw/irdma/hw.c | 28 ++++++++++++++++------------
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c
index 91c0e7298283..bc30e465c670 100644
--- a/drivers/infiniband/hw/irdma/cm.c
+++ b/drivers/infiniband/hw/irdma/cm.c
@@ -3281,6 +3281,7 @@ int irdma_setup_cm_core(struct irdma_device *iwdev, u8 rdma_ver)
 	spin_lock_init(&cm_core->ht_lock);
 	spin_lock_init(&cm_core->listen_list_lock);
 	spin_lock_init(&cm_core->apbvt_lock);
+	mutex_init(&cm_core->apbvt_add_mutex);
 	switch (rdma_ver) {
 	case IRDMA_GEN_1:
 		cm_core->form_cm_frame = irdma_form_uda_cm_frame;
diff --git a/drivers/infiniband/hw/irdma/cm.h b/drivers/infiniband/hw/irdma/cm.h
index 48ee285cf745..6c5f4335f09d 100644
--- a/drivers/infiniband/hw/irdma/cm.h
+++ b/drivers/infiniband/hw/irdma/cm.h
@@ -357,7 +357,8 @@ struct irdma_cm_core {
 	struct workqueue_struct *event_wq;
 	spinlock_t ht_lock; /* protect CM node (active side) list */
 	spinlock_t listen_list_lock; /* protect listener list */
-	spinlock_t apbvt_lock; /*serialize apbvt add/del entries*/
+	spinlock_t apbvt_lock; /* serialize APBVT entry access */
+	struct mutex apbvt_add_mutex; /* serialize APBVT hardware adds */
 	u64 stats_nodes_created;
 	u64 stats_nodes_destroyed;
 	u64 stats_listen_created;
diff --git a/drivers/infiniband/hw/irdma/hw.c b/drivers/infiniband/hw/irdma/hw.c
index c345cc654256..23ad18167607 100644
--- a/drivers/infiniband/hw/irdma/hw.c
+++ b/drivers/infiniband/hw/irdma/hw.c
@@ -2474,29 +2474,33 @@ struct irdma_apbvt_entry *irdma_add_apbvt(struct irdma_device *iwdev, u16 port)
 	struct irdma_apbvt_entry *entry;
 	unsigned long flags;
 
+	mutex_lock(&cm_core->apbvt_add_mutex);
 	spin_lock_irqsave(&cm_core->apbvt_lock, flags);
 	entry = irdma_lookup_apbvt_entry(cm_core, port);
-	if (entry) {
-		spin_unlock_irqrestore(&cm_core->apbvt_lock, flags);
-		return entry;
-	}
+	spin_unlock_irqrestore(&cm_core->apbvt_lock, flags);
+	if (entry)
+		goto out;
 
-	entry = kzalloc_obj(*entry, GFP_ATOMIC);
-	if (!entry) {
-		spin_unlock_irqrestore(&cm_core->apbvt_lock, flags);
-		return NULL;
-	}
+	entry = kzalloc_obj(*entry, GFP_KERNEL);
+	if (!entry)
+		goto out;
 
 	entry->port = port;
 	entry->use_cnt = 1;
-	hash_add(cm_core->apbvt_hash_tbl, &entry->hlist, entry->port);
-	spin_unlock_irqrestore(&cm_core->apbvt_lock, flags);
 
 	if (irdma_cqp_manage_apbvt_cmd(iwdev, port, true)) {
 		kfree(entry);
-		return NULL;
+		entry = NULL;
+		goto out;
 	}
 
+	spin_lock_irqsave(&cm_core->apbvt_lock, flags);
+	hash_add(cm_core->apbvt_hash_tbl, &entry->hlist, entry->port);
+	spin_unlock_irqrestore(&cm_core->apbvt_lock, flags);
+
+out:
+	mutex_unlock(&cm_core->apbvt_add_mutex);
+
 	return entry;
 }
 
-- 
2.43.0

Re: [PATCH] RDMA/irdma: publish APBVT entries after hardware setup

[sashiko-bot]

> irdma_add_apbvt() inserts a new entry into apbvt_hash_tbl before
> sending the CQP command that creates the hardware entry. If the command
> fails, the entry is freed without being removed from the hash table. A
> later lookup of the same port traverses the freed entry.
> 
> Removing the entry on failure is not sufficient because another thread
> can find it and increment use_cnt while the CQP command is in progress.
> 
> Serialize APBVT additions and publish new entries only after the hardware
> command succeeds. Since the allocation no longer runs under apbvt_lock,
> use GFP_KERNEL.
> 
> The issue was found by static analysis and confirmed with a KASAN
> reproducer that forces the CQP failure and retries the same port.
> 
> [ ... ]
>  init_module+0x210/0xff0 [irdma_apbvt_repro]
> 
> Fixes: 44d9e52977a1 ("RDMA/irdma: Implement device initialization definitions")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jaeyeong Lee <lee@jaeyeong.cc>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111092505.3121847-1-lee@jaeyeong.cc?part=1

Re: [PATCH] RDMA/irdma: publish APBVT entries after hardware setup

[Jacob Moroni]

Thanks. Looks good.

> +       entry = kzalloc_obj(*entry, GFP_KERNEL);
> +       if (!entry)
> +               goto out;

I think the GFP_KERNEL argument can be omitted here.

Reviewed-by: Jacob Moroni <jmoroni@google.com>