[PATCH for-rc v2 1/8] RDMA/bnxt_re: Reject executable mappings of the DBR and toggle pages

[PATCH for-rc v2 1/8] RDMA/bnxt_re: Reject executable mappings of the DBR and toggle pages

[Selvin Xavier]

The BNXT_RE_MMAP_DBR_PAGE and BNXT_RE_MMAP_TOGGLE_PAGE cases of
bnxt_re_mmap() hand out kernel pages that userspace is only supposed to
read. VM_WRITE was already rejected, but VM_EXEC was not, so userspace
could map these kernel pages executable. Reject VM_EXEC as well.
Also, return EPERM instead of EFAULT.

Fixes: 9b66c9af7172 ("RDMA/bnxt_re: Clear VM_MAYWRITE on DBR/toggle page mmap")
CC: Yousef Alhouseen <alhouseenyousef@gmail.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index ccd2702db78b..d80a22e4ac70 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -5057,9 +5057,9 @@ int bnxt_re_mmap(struct ib_ucontext *ib_uctx, struct vm_area_struct *vma)
 		break;
 	case BNXT_RE_MMAP_DBR_PAGE:
 	case BNXT_RE_MMAP_TOGGLE_PAGE:
-		/* Driver doesn't expect write access for user space */
-		if (vma->vm_flags & VM_WRITE) {
-			ret = -EFAULT;
+		/* Driver doesn't expect write and exec access for user space */
+		if (vma->vm_flags & (VM_WRITE | VM_EXEC)) {
+			ret = -EPERM;
 		} else {
 			vm_flags_clear(vma, VM_MAYWRITE);
 			ret = vm_insert_page(vma, vma->vm_start,
-- 
2.39.3

Re: [PATCH for-rc v2 1/8] RDMA/bnxt_re: Reject executable mappings of the DBR and toggle pages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch rejects VM_EXEC to prevent executable mappings of the DBR and toggle pages, but fails to clear VM_MAYEXEC. This allows userspace to bypass the restriction via mprotect().
--

commit b5ba6f660e81b67fecc55a7c6ceae5f0724ddc0a
Author: Selvin Xavier <selvin.xavier@broadcom.com>

RDMA/bnxt_re: Reject executable mappings of the DBR and toggle pages

This commit updates bnxt_re_mmap() to reject VM_EXEC mappings for DBR and
toggle pages, returning -EPERM instead of -EFAULT. It is intended to prevent
userspace from mapping these pages as executable.

> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index ccd2702db78b5..d80a22e4ac70d 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -5057,9 +5057,9 @@ int bnxt_re_mmap(struct ib_ucontext *ib_uctx, struct vm_area_struct *vma)
>  		break;
>  	case BNXT_RE_MMAP_DBR_PAGE:
>  	case BNXT_RE_MMAP_TOGGLE_PAGE:
> -		/* Driver doesn't expect write access for user space */
> -		if (vma->vm_flags & VM_WRITE) {
> -			ret = -EFAULT;
> +		/* Driver doesn't expect write and exec access for user space */
> +		if (vma->vm_flags & (VM_WRITE | VM_EXEC)) {
> +			ret = -EPERM;
>  		} else {
>  			vm_flags_clear(vma, VM_MAYWRITE);

[Severity: Medium]
Does this code also need to clear VM_MAYEXEC? While the patch correctly
rejects an initial mapping with PROT_EXEC, failing to clear VM_MAYEXEC allows
userspace to bypass the restriction by calling mprotect() with PROT_EXEC
after the mmap() succeeds.

>  			ret = vm_insert_page(vma, vma->vm_start,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=1

[PATCH for-rc v2 2/8] RDMA/bnxt_re: Detect wrong sge_len passed for inline

[Selvin Xavier]

Avoid handling wrong sge_len by adding extra check
to see if the passed length is more than the inline
size supported.

Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index d80a22e4ac70..e56830a669bf 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -3168,8 +3168,9 @@ static int bnxt_re_copy_inline_data(struct bnxt_re_dev *rdev,
 				wr->sg_list[i].addr;
 		sge_len = wr->sg_list[i].length;
 
-		if ((sge_len + wqe->inline_len) >
-		    BNXT_QPLIB_SWQE_MAX_INLINE_LENGTH) {
+		if (sge_len > BNXT_QPLIB_SWQE_MAX_INLINE_LENGTH ||
+		    ((sge_len + wqe->inline_len) >
+		    BNXT_QPLIB_SWQE_MAX_INLINE_LENGTH)) {
 			ibdev_err(&rdev->ibdev,
 				  "Inline data size requested > supported value");
 			return -EINVAL;
-- 
2.39.3

Re: [PATCH for-rc v2 2/8] RDMA/bnxt_re: Detect wrong sge_len passed for inline

[sashiko-bot]

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

Pre-existing issues:
- [High] Integer overflow in `bnxt_re_build_sgl` leads to improper rejection of valid large RDMA payloads.
--

commit ce7a03605a13da6e57de1f53a8c0119ccee546dd
Author: Selvin Xavier <selvin.xavier@broadcom.com>

RDMA/bnxt_re: Detect wrong sge_len passed for inline

This commit avoids handling wrong sge_len by adding an extra check to see if
the passed length is more than the inline size supported.

> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index d80a22e4ac70d..e56830a669bf4 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -3168,8 +3168,9 @@ static int bnxt_re_copy_inline_data(struct bnxt_re_dev *rdev,
>  				wr->sg_list[i].addr;
>  		sge_len = wr->sg_list[i].length;
>  
> -		if ((sge_len + wqe->inline_len) >
> -		    BNXT_QPLIB_SWQE_MAX_INLINE_LENGTH) {
> +		if (sge_len > BNXT_QPLIB_SWQE_MAX_INLINE_LENGTH ||
> +		    ((sge_len + wqe->inline_len) >
> +		    BNXT_QPLIB_SWQE_MAX_INLINE_LENGTH)) {
>  			ibdev_err(&rdev->ibdev,
>  				  "Inline data size requested > supported value");
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but it exists in the directly associated code
paths. Does bnxt_re_build_sgl() have an integer overflow when dealing with
large RDMA payloads?

drivers/infiniband/hw/bnxt_re/ib_verbs.c:bnxt_re_build_sgl() {
        int i, total = 0;

        for (i = 0; i < num; i++) {
                ...
                total += sg_list[i].size;
        }
        return total;
}

Since total is declared as a signed 32-bit integer, could summing unsigned
32-bit size values from multiple SGEs cause it to overflow and become
negative for payloads greater than 2GB?

If this overflows and becomes negative, it appears the negative total is
propagated here:

drivers/infiniband/hw/bnxt_re/ib_verbs.c:bnxt_re_post_send() {
        ...
        payload_sz = bnxt_re_copy_wr_payload(qp->rdev, wr, &wqe);
        if (payload_sz < 0) {
                rc = -EINVAL;
                goto bad;
        }
}

Would this cause legitimate RDMA send or write operations with payloads
between 2GB and 4GB to be erroneously rejected, breaking core functionality
for user-space applications performing large I/O?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=2

[PATCH for-rc v2 3/8] RDMA/bnxt_re: Validate num_sge in bnxt_re_post_srq_recv()

[Selvin Xavier]

Add a check if max sge for SRQ is exceeding the device
maximum.

Fixes: 37cb11acf1f7 ("RDMA/bnxt_re: Add SRQ support for Broadcom adapters")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index e56830a669bf..ab9d4d6bf5b0 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -2389,6 +2389,13 @@ int bnxt_re_post_srq_recv(struct ib_srq *ib_srq, const struct ib_recv_wr *wr,
 	while (wr) {
 		/* Transcribe each ib_recv_wr to qplib_swqe */
 		wqe.num_sge = wr->num_sge;
+		if (wr->num_sge > srq->qplib_srq.max_sge) {
+			ibdev_err(&srq->rdev->ibdev,
+				  "Limit exceeded for SRQ Receive SGEs");
+			rc = -EINVAL;
+			*bad_wr = wr;
+			break;
+		}
 		bnxt_re_build_sgl(wr->sg_list, wqe.sg_list, wr->num_sge);
 		wqe.wr_id = wr->wr_id;
 		wqe.type = BNXT_QPLIB_SWQE_TYPE_RECV;
-- 
2.39.3

Re: [PATCH for-rc v2 3/8] RDMA/bnxt_re: Validate num_sge in bnxt_re_post_srq_recv()

[sashiko-bot]

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

New issues:
- [Medium] Unconditional error logging on fast-path enables unprivileged log spam (Denial of Service)

Pre-existing issues:
- [High] Uninitialized stack memory in hardware work queue descriptor (wqe.flags)
--

commit a50a4d21bcd2c8e40025a33fa29a7f621d05a408
Author: Selvin Xavier <selvin.xavier@broadcom.com>

RDMA/bnxt_re: Validate num_sge in bnxt_re_post_srq_recv()

This commit adds a validation check in bnxt_re_post_srq_recv() to ensure
that the number of scatter/gather elements (SGE) provided for a Shared
Receive Queue (SRQ) does not exceed the device's configured maximum.

> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index e56830a669bf4..ab9d4d6bf5b02 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -2386,9 +2386,16 @@ int bnxt_re_post_srq_recv(struct ib_srq *ib_srq, const struct ib_recv_wr *wr,
>  			  const struct ib_recv_wr **bad_wr)
>  {
>  	struct bnxt_re_srq *srq = container_of(ib_srq, struct bnxt_re_srq,
>  					       ib_srq);
>  	struct bnxt_qplib_swqe wqe;

[Severity: High]
This is a pre-existing issue, but does this allocation leave wqe.flags
uninitialized before the structure is passed to the hardware?

Any legitimate call to bnxt_re_post_srq_recv allocates the wqe structure on
the stack without zeroing it. The function sets specific fields but appears
to leave wqe.flags uninitialized before calling bnxt_qplib_post_srq_recv,
which writes the flags field into the hardware's ring buffer descriptor.

Can this result in unpredictable hardware behavior or dropped work requests
if the garbage stack data is misinterpreted by the hardware datapath?

>  	unsigned long flags;
>  	int rc = 0;
>  
>  	spin_lock_irqsave(&srq->lock, flags);
>  	while (wr) {
>  		/* Transcribe each ib_recv_wr to qplib_swqe */
>  		wqe.num_sge = wr->num_sge;
> +		if (wr->num_sge > srq->qplib_srq.max_sge) {
> +			ibdev_err(&srq->rdev->ibdev,
> +				  "Limit exceeded for SRQ Receive SGEs");

[Severity: Medium]
Since this function is on a fast-path datapath reachable from unprivileged
userspace via the RDMA uverbs API, could this unconditional error log lead
to log spam?

An unprivileged userspace application could intentionally flood the kernel
ring buffer by passing a wr->num_sge that is valid for the system call but
exceeds the driver's max_sge limit. 

Would it be better to rate-limit this ibdev_err call, or remove it entirely
to prevent potential denial of service through disk space exhaustion and
system performance degradation?

> +			rc = -EINVAL;
> +			*bad_wr = wr;
> +			break;
> +		}
>  		bnxt_re_build_sgl(wr->sg_list, wqe.sg_list, wr->num_sge);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=3

[PATCH for-rc v2 4/8] RDMA/bnxt_re: Validate SRQ max_sge at create time

[Selvin Xavier]

bnxt_re_create_srq() validates attr.max_wr but copies attr.max_sge
into srq->qplib_srq.max_sge unvalidated. This lets an unprivileged
user request an arbitrarily large max_sge, defeating the num_sge
check added in bnxt_re_post_srq_recv() and overflowing the fixed
13-entry (BNXT_VAR_MAX_SGE) sg_list[] array in bnxt_re_build_sgl().

Reject max_sge > dev_attr->max_srq_sges at create time, as create_qp
already does for QP send/recv SGEs. Also clamp max_srq_sges itself in
bnxt_qplib_get_dev_attr() to BNXT_STATIC_MAX_SGE (6), the actual max
SGEs an SRQ ring entry supports, since it was taken straight from
firmware with no bound.

Fixes: 37cb11acf1f7 ("RDMA/bnxt_re: Add SRQ support for Broadcom adapters")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 8 ++++++++
 drivers/infiniband/hw/bnxt_re/qplib_sp.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index ab9d4d6bf5b0..72c098b48826 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -2235,6 +2235,14 @@ int bnxt_re_create_srq(struct ib_srq *ib_srq,
 		goto exit;
 	}
 
+	if (srq_init_attr->attr.max_sge > dev_attr->max_srq_sges) {
+		ibdev_err(&rdev->ibdev,
+			  "Create SRQ failed - max_sge %d exceeds supported %d",
+			  srq_init_attr->attr.max_sge, dev_attr->max_srq_sges);
+		rc = -EINVAL;
+		goto exit;
+	}
+
 	if (srq_init_attr->srq_type != IB_SRQT_BASIC) {
 		rc = -EOPNOTSUPP;
 		goto exit;
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_sp.c b/drivers/infiniband/hw/bnxt_re/qplib_sp.c
index ec9eb52a8ebf..9aaa2b5204b8 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_sp.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_sp.c
@@ -160,7 +160,7 @@ int bnxt_qplib_get_dev_attr(struct bnxt_qplib_rcfw *rcfw)
 
 	attr->max_srq = le16_to_cpu(sb->max_srq);
 	attr->max_srq_wqes = le32_to_cpu(sb->max_srq_wr) - 1;
-	attr->max_srq_sges = sb->max_srq_sge;
+	attr->max_srq_sges = min_t(u32, sb->max_srq_sge, BNXT_STATIC_MAX_SGE);
 	attr->max_pkey = 1;
 	attr->max_inline_data = attr->max_qp_sges * sizeof(struct sq_sge);
 	if (!bnxt_qplib_is_chip_gen_p7(rcfw->res->cctx))
-- 
2.39.3

Re: [PATCH for-rc v2 4/8] RDMA/bnxt_re: Validate SRQ max_sge at create time

[sashiko-bot]

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

Pre-existing issues:
- [High] Silent 16-bit truncation of SRQ size when submitting SRQ creation command to firmware.
--

commit 0a4f291b3aa3f286f901471db25ab33de33a632f
Author: Selvin Xavier <selvin.xavier@broadcom.com>

RDMA/bnxt_re: Validate SRQ max_sge at create time

bnxt_re_create_srq() validates attr.max_wr but copies attr.max_sge
into srq->qplib_srq.max_sge unvalidated. This lets an unprivileged
user request an arbitrarily large max_sge, defeating the num_sge
check added in bnxt_re_post_srq_recv() and overflowing the fixed
13-entry (BNXT_VAR_MAX_SGE) sg_list[] array in bnxt_re_build_sgl().

> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index ab9d4d6bf5b02..72c098b488263 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -2235,6 +2235,14 @@ int bnxt_re_create_srq(struct ib_srq *ib_srq,
>  		goto exit;
>  	}
>  
> +	if (srq_init_attr->attr.max_sge > dev_attr->max_srq_sges) {
> +		ibdev_err(&rdev->ibdev,
> +			  "Create SRQ failed - max_sge %d exceeds supported %d",
> +			  srq_init_attr->attr.max_sge, dev_attr->max_srq_sges);
> +		rc = -EINVAL;
> +		goto exit;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but while looking at SRQ size validation,
could large max_wr values cause a truncation issue when passed to
the firmware?

If unprivileged userspace calls ibv_create_srq() with a large max_wr value
permitted by the 32-bit max_srq_wr hardware capability, the driver rounds
up the requested queue depth to a power of two and stores it in the 32-bit
srq->hwq.max_elements.

In bnxt_qplib_create_srq(), this value is blindly cast to a 16-bit integer
for the firmware command:

drivers/infiniband/hw/bnxt_re/qplib_fp.c:bnxt_qplib_create_srq() {
    ...
    req.srq_size = cpu_to_le16((u16)srq->hwq.max_elements);
    ...
}

An exact multiple of 65536 will truncate to 0. The firmware is instructed
to create a 0-sized SRQ, while the kernel and userspace map a large queue.
Ringing the doorbell after writing to this queue will cause the firmware to
access unallocated memory or crash.

Does this hardware capability or queue depth need to be capped at 16-bits
during SRQ creation to avoid this mismatch?

>  	if (srq_init_attr->srq_type != IB_SRQT_BASIC) {
>  		rc = -EOPNOTSUPP;
>  		goto exit;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=4

[PATCH for-rc v2 5/8] RDMA/bnxt_re: Fix rdev lifetime races in suspend/resume/shutdown

[Selvin Xavier]

en_info->rdev is read by unlocked L2 ULP callbacks
(bnxt_re_stop_irq/start_irq/async_notifier), which rely on rdev
being cleared before it is freed, not after. bnxt_re_add_device()
cleared it too late on a bnxt_re_dev_init() failure and
bnxt_re_resume() didn't check the return value, risking a
use-after-free/NULL deref. bnxt_re_suspend() froze and freed rdev
before clearing en_info->rdev, opening the same UAF window.
bnxt_re_shutdown() took no lock, never checked rdev for NULL, never
cleared en_info->rdev, and never freed rdev at all.

Fix the ordering in bnxt_re_add_device()/bnxt_re_suspend(), add the
missing return-value check in bnxt_re_resume(), and rewrite
bnxt_re_shutdown() to take the lock, check for NULL, and reuse
bnxt_re_remove_device() with the same clear-before-free ordering.

Fixes: dee3da3422d5 ("RDMA/bnxt_re: Change aux driver data to en_info to hold more information")
Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 32 ++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 17654a9e23fe..91c2edebdb93 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -2433,11 +2433,14 @@ static int bnxt_re_add_device(struct auxiliary_device *adev, u8 op_type)
 	bnxt_re_update_en_info_rdev(rdev, en_info, adev);
 
 	rc = bnxt_re_dev_init(rdev, op_type);
-	if (rc)
+	if (rc) {
+		bnxt_re_update_en_info_rdev(NULL, en_info, adev);
 		goto re_dev_dealloc;
+	}
 
 	rc = bnxt_re_ib_init(rdev);
 	if (rc) {
+		bnxt_re_update_en_info_rdev(NULL, en_info, adev);
 		pr_err("Failed to register with IB: %s",
 			aux_priv->aux_dev.name);
 		goto re_dev_uninit;
@@ -2448,7 +2451,6 @@ static int bnxt_re_add_device(struct auxiliary_device *adev, u8 op_type)
 	return 0;
 
 re_dev_uninit:
-	bnxt_re_update_en_info_rdev(NULL, en_info, adev);
 	bnxt_re_dev_uninit(rdev, BNXT_RE_COMPLETE_REMOVE);
 re_dev_dealloc:
 	ib_dealloc_device(&rdev->ibdev);
@@ -2517,9 +2519,13 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
 	struct bnxt_en_dev *en_dev;
 	struct bnxt_re_dev *rdev;
 
+	mutex_lock(&bnxt_re_mutex);
 	rdev = en_info->rdev;
+	if (!rdev) {
+		mutex_unlock(&bnxt_re_mutex);
+		return 0;
+	}
 	en_dev = en_info->en_dev;
-	mutex_lock(&bnxt_re_mutex);
 
 	ibdev_info(&rdev->ibdev, "Handle device suspend call");
 	/* Check the current device state from bnxt_en_dev and move the
@@ -2539,8 +2545,9 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
 
 	ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
 		   __func__, en_dev->en_state);
-	bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
+
 	bnxt_re_update_en_info_rdev(NULL, en_info, adev);
+	bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
 	mutex_unlock(&bnxt_re_mutex);
 
 	return 0;
@@ -2550,9 +2557,14 @@ static int bnxt_re_resume(struct auxiliary_device *adev)
 {
 	struct bnxt_re_en_dev_info *en_info = auxiliary_get_drvdata(adev);
 	struct bnxt_re_dev *rdev;
+	int rc;
 
 	mutex_lock(&bnxt_re_mutex);
-	bnxt_re_add_device(adev, BNXT_RE_POST_RECOVERY_INIT);
+	rc = bnxt_re_add_device(adev, BNXT_RE_POST_RECOVERY_INIT);
+	if (rc) {
+		mutex_unlock(&bnxt_re_mutex);
+		return rc;
+	}
 	rdev = en_info->rdev;
 	ibdev_info(&rdev->ibdev, "Device resume completed");
 	mutex_unlock(&bnxt_re_mutex);
@@ -2565,9 +2577,15 @@ static void bnxt_re_shutdown(struct auxiliary_device *adev)
 	struct bnxt_re_en_dev_info *en_info = auxiliary_get_drvdata(adev);
 	struct bnxt_re_dev *rdev;
 
+	mutex_lock(&bnxt_re_mutex);
 	rdev = en_info->rdev;
-	ib_unregister_device(&rdev->ibdev);
-	bnxt_re_dev_uninit(rdev, BNXT_RE_COMPLETE_REMOVE);
+	if (!rdev)
+		goto out;
+
+	bnxt_re_update_en_info_rdev(NULL, en_info, adev);
+	bnxt_re_remove_device(rdev, BNXT_RE_COMPLETE_REMOVE, adev);
+out:
+	mutex_unlock(&bnxt_re_mutex);
 }
 
 static const struct auxiliary_device_id bnxt_re_id_table[] = {
-- 
2.39.3

Re: [PATCH for-rc v2 5/8] RDMA/bnxt_re: Fix rdev lifetime races in suspend/resume/shutdown

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch misses applying the Use-After-Free fix to `bnxt_re_remove()`, leaving the exact same race window open during device unbind or module unload.
- [High] Use-After-Free during `bnxt_re_dev_init()` failure path due to internal resource teardown while `en_info->rdev` is still valid.
- [High] Insufficient synchronization between `bnxt_re_update_en_info_rdev()` (`rtnl_lock`) and lockless RCU readers like `bnxt_re_async_notifier()`, leading to Use-After-Free.
--

commit 1f3564eb53548d58fabdef6b5b5a607d2ac66cdf
Author: Selvin Xavier <selvin.xavier@broadcom.com>
Subject: RDMA/bnxt_re: Fix rdev lifetime races in suspend/resume/shutdown

This commit attempts to fix use-after-free windows in the suspend, resume,
and shutdown paths by ensuring the rdev pointer is cleared before device
resources are freed.

> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index ce72db1b4bc34..142bf9a8ca4a4 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -2427,11 +2427,14 @@ static int bnxt_re_add_device(struct auxiliary_device *adev, u8 op_type)
>  	bnxt_re_update_en_info_rdev(rdev, en_info, adev);
>  
>  	rc = bnxt_re_dev_init(rdev, op_type);
> -	if (rc)
> +	if (rc) {
> +		bnxt_re_update_en_info_rdev(NULL, en_info, adev);
>  		goto re_dev_dealloc;
> +	}

[Severity: High]
This is a pre-existing issue, but does this approach fully prevent the race
during a device initialization failure?

If bnxt_re_dev_init() fails internally after registering the netdev, it
jumps to its internal fail label and calls bnxt_re_dev_uninit(). At this
stage, en_info->rdev is still valid.

If an async ULP callback triggers during this teardown, could it access
partially destroyed resources before bnxt_re_dev_init() returns and the
pointer is finally cleared here?

[ ... ]

> @@ -2533,8 +2539,9 @@ static int bnxt_re_suspend(struct auxiliary_device *adev, pm_message_t state)
>  
>  	ibdev_info(&rdev->ibdev, "%s: L2 driver notified to stop en_state 0x%lx",
>  		   __func__, en_dev->en_state);
> -	bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
> +
> +	bnxt_re_update_en_info_rdev(NULL, en_info, adev);
> +	bnxt_re_remove_device(rdev, BNXT_RE_PRE_RECOVERY_REMOVE, adev);
>  	mutex_unlock(&bnxt_re_mutex);

[Severity: High]
This is a pre-existing issue, but is there sufficient synchronization between
the pointer clearing and lockless RCU readers?

While bnxt_re_update_en_info_rdev() uses rtnl_lock() to clear the pointer,
readers like bnxt_re_async_notifier() read it under rcu_read_lock().

Because bnxt_re_remove_device() subsequently destroys the workqueue via
bnxt_re_uninit_dcb_wq() without waiting for synchronize_rcu(), could an
active concurrent RCU reader proceed to call queue_work() on the destroyed
workqueue?


Looking at the rest of the file, it appears a similar pattern exists in
bnxt_re_remove() which is not covered by this patch:

drivers/infiniband/hw/bnxt_re/main.c:bnxt_re_remove() {
        ...
        rdev = en_info->rdev;

        if (rdev)
                bnxt_re_remove_device(rdev, BNXT_RE_COMPLETE_REMOVE, adev);
        ...
}

[Severity: High]
This is a pre-existing issue, but since the patch fixes the use-after-free
window during suspend, resume, and shutdown, does bnxt_re_remove() need the
same fix?

When the auxiliary device is removed, bnxt_re_remove() calls
bnxt_re_remove_device() to destroy resources without first clearing
en_info->rdev. Could an asynchronous L2 ULP callback fire concurrently and
access the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=5

[PATCH for-rc v2 6/8] RDMA/bnxt_re: Fix the PD and DPI table size

[Selvin Xavier]

The PD and DPI bitmaps were sized as max >> 3 (bytes),
but bitmap ops (set_bit(), clear_bit(), find_first_bit(),
test_and_set_bit()) operate on whole unsigned long words,
so whenever max isn't a multiple of BITS_PER_LONG,
the buffer under-allocates and the top word's bitops
read/write past the end of the kmalloc()'d buffer.
Most exposed on the DPI table, since dpit->max comes from the
firmware-reported dev_attr->max_dpi with no alignment guarantee.

Fix the size both allocations with BITS_TO_LONGS(max) * sizeof(unsigned long).

Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_res.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c
index 756f8b5f042a..7ff587ce9126 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
@@ -45,6 +45,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/if_vlan.h>
 #include <linux/vmalloc.h>
+#include <linux/bitops.h>
 #include <rdma/ib_verbs.h>
 #include <rdma/iter.h>
 
@@ -668,9 +669,9 @@ static int bnxt_qplib_alloc_pd_tbl(struct bnxt_qplib_res *res,
 {
 	u32 bytes;
 
-	bytes = max >> 3;
+	bytes = BITS_TO_LONGS(max) * sizeof(unsigned long);
 	if (!bytes)
-		bytes = 1;
+		bytes = sizeof(unsigned long);
 	pdt->tbl = kmalloc(bytes, GFP_KERNEL);
 	if (!pdt->tbl)
 		return -ENOMEM;
@@ -848,9 +849,9 @@ static int bnxt_qplib_alloc_dpi_tbl(struct bnxt_qplib_res *res,
 	if (!dpit->app_tbl)
 		return -ENOMEM;
 
-	bytes = dpit->max >> 3;
+	bytes = BITS_TO_LONGS(dpit->max) * sizeof(unsigned long);
 	if (!bytes)
-		bytes = 1;
+		bytes = sizeof(unsigned long);
 
 	dpit->tbl = kmalloc(bytes, GFP_KERNEL);
 	if (!dpit->tbl) {
-- 
2.39.3

Re: [PATCH for-rc v2 6/8] RDMA/bnxt_re: Fix the PD and DPI table size

[sashiko-bot]

> The PD and DPI bitmaps were sized as max >> 3 (bytes),
> but bitmap ops (set_bit(), clear_bit(), find_first_bit(),
> test_and_set_bit()) operate on whole unsigned long words,
> so whenever max isn't a multiple of BITS_PER_LONG,
> the buffer under-allocates and the top word's bitops
> read/write past the end of the kmalloc()'d buffer.
> Most exposed on the DPI table, since dpit->max comes from the
> firmware-reported dev_attr->max_dpi with no alignment guarantee.
> 
> Fix the size both allocations with BITS_TO_LONGS(max) * sizeof(unsigned long).
> 
> Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=6

[PATCH for-rc v2 7/8] RDMA/bnxt_re: Use bnxt_ext_stats_supported for counter count selection

[Selvin Xavier]

Selecting between BNXT_RE_NUM_EXT_COUNTERS and
BNXT_RE_NUM_STD_COUNTERS relied only on
bnxt_qplib_is_chip_gen_p5_p7(), which doesn't account for the
extended stats capability flag or the PF/VF restriction. Use
bnxt_ext_stats_supported() instead, matching the check already
used to populate the extended stats, so the counter count stays
consistent with what gets filled in.

Fixes: 8238c7bd8420 ("RDMA/bnxt_re: Fix the statistics for Gen P7 VF")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/hw_counters.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/hw_counters.c b/drivers/infiniband/hw/bnxt_re/hw_counters.c
index 651cf9d0e0c7..575b97030587 100644
--- a/drivers/infiniband/hw/bnxt_re/hw_counters.c
+++ b/drivers/infiniband/hw/bnxt_re/hw_counters.c
@@ -413,7 +413,8 @@ int bnxt_re_ib_get_hw_stats(struct ib_device *ibdev,
 	}
 
 done:
-	return bnxt_qplib_is_chip_gen_p5_p7(rdev->chip_ctx) ?
+	return bnxt_ext_stats_supported(rdev->chip_ctx, rdev->dev_attr->dev_cap_flags,
+					 rdev->is_virtfn) ?
 		BNXT_RE_NUM_EXT_COUNTERS : BNXT_RE_NUM_STD_COUNTERS;
 }
 
@@ -423,7 +424,8 @@ struct rdma_hw_stats *bnxt_re_ib_alloc_hw_port_stats(struct ib_device *ibdev,
 	struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev);
 	int num_counters = 0;
 
-	if (bnxt_qplib_is_chip_gen_p5_p7(rdev->chip_ctx))
+	if (bnxt_ext_stats_supported(rdev->chip_ctx, rdev->dev_attr->dev_cap_flags,
+				     rdev->is_virtfn))
 		num_counters = BNXT_RE_NUM_EXT_COUNTERS;
 	else
 		num_counters = BNXT_RE_NUM_STD_COUNTERS;
-- 
2.39.3

Re: [PATCH for-rc v2 7/8] RDMA/bnxt_re: Use bnxt_ext_stats_supported for counter count selection

[sashiko-bot]

> Selecting between BNXT_RE_NUM_EXT_COUNTERS and
> BNXT_RE_NUM_STD_COUNTERS relied only on
> bnxt_qplib_is_chip_gen_p5_p7(), which doesn't account for the
> extended stats capability flag or the PF/VF restriction. Use
> bnxt_ext_stats_supported() instead, matching the check already
> used to populate the extended stats, so the counter count stays
> consistent with what gets filled in.
> 
> Fixes: 8238c7bd8420 ("RDMA/bnxt_re: Fix the statistics for Gen P7 VF")
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=7

[PATCH for-rc v2 8/8] RDMA/bnxt_re: Check is_in_used before trusting RCFW completion

[Selvin Xavier]

Gate the crsqe->resp memcpy in bnxt_qplib_process_qp_event() on
is_in_used in addition to is_waiter_alive, since a late/duplicate
completion for an already-retired cookie could still write through a
stale resp pointer.

Also disarm the waiter and clear resp when a wait call bails out early
on ERR_DEVICE_DETACHED, and require is_in_used before re-triggering
__destroy_timedout_ah(), closing the same stale-cookie exposure
elsewhere in the completion path.

Fixes: 691eb7c6110f ("RDMA/bnxt_re: handle command completions after driver detect a timedout")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
index 9ee0d500436c..0339e55bea88 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
@@ -511,14 +511,16 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
 	else
 		rc = __poll_for_resp(rcfw, cookie);
 
-	if (rc) {
+	if (rc || test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags)) {
 		spin_lock_bh(&rcfw->cmdq.hwq.lock);
 		crsqe = &rcfw->crsqe_tbl[cookie];
 		crsqe->is_waiter_alive = false;
+		crsqe->resp = NULL;
 		if (rc == -ENODEV)
 			set_bit(FIRMWARE_STALL_DETECTED, &rcfw->cmdq.flags);
 		spin_unlock_bh(&rcfw->cmdq.hwq.lock);
-		return -ETIMEDOUT;
+		if (rc)
+			return -ETIMEDOUT;
 	}
 
 	if (evnt->status) {
@@ -627,6 +629,7 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
 	struct bnxt_qplib_qp *qp;
 	u16 cookie, blocked = 0;
 	bool is_waiter_alive;
+	bool is_in_used;
 	struct pci_dev *pdev;
 	u32 wait_cmds = 0;
 	int rc = 0;
@@ -682,7 +685,7 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
 		if (crsqe->is_internal_cmd && !qp_event->status)
 			atomic_dec(&rcfw->timeout_send);
 
-		if (crsqe->is_waiter_alive) {
+		if (crsqe->is_waiter_alive && crsqe->is_in_used) {
 			if (crsqe->resp) {
 				memcpy(crsqe->resp, qp_event, sizeof(*qp_event));
 				/* Insert write memory barrier to ensure that
@@ -697,6 +700,7 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
 
 		req_size = crsqe->req_size;
 		is_waiter_alive = crsqe->is_waiter_alive;
+		is_in_used = crsqe->is_in_used;
 
 		crsqe->req_size = 0;
 		if (!is_waiter_alive)
@@ -718,7 +722,7 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
 		 * resource leak and relatively not critical +  unlikely
 		 * scenario. Current design is not to handle such case.
 		 */
-		if (!is_waiter_alive && !qp_event->status &&
+		if (is_in_used && !is_waiter_alive && !qp_event->status &&
 		    qp_event->event == CREQ_QP_EVENT_EVENT_CREATE_AH)
 			__destroy_timedout_ah(rcfw,
 					      (struct creq_create_ah_resp *)
-- 
2.39.3

Re: [PATCH for-rc v2 8/8] RDMA/bnxt_re: Check is_in_used before trusting RCFW completion

[sashiko-bot]

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

Pre-existing issues:
- [High] Duplicate completions for retired internal commands bypass the is_in_used gate, causing timeout_send atomic underflow.
--

commit a5d91fba60b82a58962973687dc528a6bc7de745
Author: Selvin Xavier <selvin.xavier@broadcom.com>

RDMA/bnxt_re: Check is_in_used before trusting RCFW completion

This commit gates crsqe->resp memory copies and timed-out AH destruction on
is_in_used to prevent operating on stale responses. It also disarms waiters
and clears the response pointer when bailing out early on a detached device.

> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> index 9ee0d500436cb..0339e55bea886 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
[ ... ]
> @@ -682,7 +685,7 @@ static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
>  		if (crsqe->is_internal_cmd && !qp_event->status)
>  			atomic_dec(&rcfw->timeout_send);

[Severity: High]
This isn't a bug introduced by this patch, but does this atomic_dec() in
bnxt_qplib_process_qp_event() need the same is_in_used check that was added
below?

If a late or duplicate firmware completion arrives for an already-retired
internal command, it will bypass the is_in_used gate here. Can this execute an
erroneous atomic_dec() on a stale response and cause the timeout_send counter
to underflow?

> -		if (crsqe->is_waiter_alive) {
> +		if (crsqe->is_waiter_alive && crsqe->is_in_used) {
>  			if (crsqe->resp) {
>  				memcpy(crsqe->resp, qp_event, sizeof(*qp_event));
>  				/* Insert write memory barrier to ensure that

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135244.122747-1-selvin.xavier@broadcom.com?part=8