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

[sashiko-bot@kernel.org]

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