[PATCH] scsi: qedi: Initialize callback state before registration

[PATCH] scsi: qedi: Initialize callback state before registration

[Runyu Xiao]

qedi_get_protocol_tlv_data() can be called asynchronously by QED after
qedi_ops->register_ops(). It takes stats_lock and reads ll2_mtu, but
__qedi_probe() currently registers the callback before initializing the
mutex and assigning the default MTU on the normal probe path.

Initialize the callback-visible state before registering qedi_cb_ops. Keep
the recovery path from reinitializing state because it reuses the existing
qedi context.

Fixes: 3cc5746e5ad7 ("scsi: qedi: Initialize the stats mutex lock")
Cc: stable@vger.kernel.org

Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/scsi/qedi/qedi_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index 227ff7bd1..f394fcbb9 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2652,8 +2652,6 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
 	snprintf(host_buf, sizeof(host_buf), "host_%d", qedi->shost->host_no);
 	qedi_ops->common->set_name(qedi->cdev, host_buf);
 
-	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
-
 	memset(&params, 0, sizeof(params));
 	params.mtu = DEF_PATH_MTU + IPV6_HDR_LEN + TCP_HDR_LEN;
 	qedi->ll2_mtu = DEF_PATH_MTU;
@@ -2670,6 +2668,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
 		spin_lock_init(&qedi->task_idx_lock);
 		mutex_init(&qedi->stats_lock);
 	}
+	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
 	qedi_ops->ll2->register_cb_ops(qedi->cdev, &qedi_ll2_cb_ops, qedi);
 	qedi_ops->ll2->start(qedi->cdev, &params);
 
-- 
2.34.1

Re: [PATCH] scsi: qedi: Initialize callback state before registration

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Network packet reception in `qedi_ll2_rx` lacks bounds checks, allowing malformed packets to cause out-of-bounds reads or NULL pointer dereferences.
- [High] Race condition and missing error checks for `kthread_run()` return value lead to invalid pointer dereferences during LL2 Rx processing or module teardown.
- [High] The patch leaves `recovery_work` and `board_disable_work` uninitialized when registering `qedi_cb_ops` callbacks, exposing uninitialized work structures to asynchronous hardware events.
- [High] User-configured custom MTU settings are silently overwritten to defaults during hardware recovery.
- [High] `iscsi_host_add` exposes the device to userspace before crucial connection structures (`ep_tbl`) are allocated, creating a race condition.
- [High] The background LL2 receiver kthread and hardware subsystem are not stopped on the `__qedi_probe` error path, leading to Use-After-Free.
- [High] The error path for `iscsi_host_add` failure calls `iscsi_host_remove`, which will crash when attempting to remove an un-added host.
--

commit 7badf30e1ff5d4629509a7cbca53b6e22f3d67df
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: qedi: Initialize callback state before registration

This patch fixes a race condition where qedi_get_protocol_tlv_data() could
be called asynchronously by QED before the stats_lock and ll2_mtu were
initialized. It moves the callback registration to after these initializations
on the normal probe path.

> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 227ff7bd1bdc9..f394fcbb97e7e 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -2652,8 +2652,6 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
>  	snprintf(host_buf, sizeof(host_buf), "host_%d", qedi->shost->host_no);
>  	qedi_ops->common->set_name(qedi->cdev, host_buf);
>  
> -	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
> -
>  	memset(&params, 0, sizeof(params));
>  	params.mtu = DEF_PATH_MTU + IPV6_HDR_LEN + TCP_HDR_LEN;
>  	qedi->ll2_mtu = DEF_PATH_MTU;

[Severity: High]
This is a pre-existing issue, but are user-configured custom MTU settings
overwritten during hardware recovery here?

During a hardware recovery event, __qedi_probe() is invoked with
mode == QEDI_MODE_RECOVERY. This unconditional assignment to DEF_PATH_MTU
discards any custom MTU size (such as a 9000-byte jumbo MTU) that the user
may have configured.

> @@ -2670,6 +2668,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
>  		spin_lock_init(&qedi->task_idx_lock);
>  		mutex_init(&qedi->stats_lock);
>  	}
> +	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);

[Severity: High]
This isn't a bug introduced by this patch, but could registering the callbacks
here expose uninitialized work structures?

The qedi_cb_ops expose callback functions like
qedi_schedule_recovery_handler() to the QED core. This handler attempts to
schedule qedi->recovery_work, but INIT_DELAYED_WORK(&qedi->recovery_work, ...)
is not called until much later in the probe sequence.

If an asynchronous hardware event occurs in this window, it looks like
schedule_delayed_work() could be invoked on zero-filled memory.

>  	qedi_ops->ll2->register_cb_ops(qedi->cdev, &qedi_ll2_cb_ops, qedi);
>  	qedi_ops->ll2->start(qedi->cdev, &params);

[Severity: High]
This is a pre-existing issue, but does starting the hardware packet reception
here introduce a race condition?

If a packet arrives immediately, qedi_ll2_rx() calls
wake_up_process(qedi->ll2_recv_thread) while the thread pointer is still NULL.

Additionally, is it safe to ignore the return value of kthread_run() when it
creates qedi->ll2_recv_thread shortly after this?

If kthread_run() fails, it returns an ERR_PTR which is neither checked nor
handled. This could cause subsequent packets or kthread_stop() during teardown
to dereference the ERR_PTR.


[Severity: High]
This is a pre-existing issue, but does exposing the device to userspace before
crucial connection structures are allocated create a race condition?

In drivers/scsi/qedi/qedi_main.c:__qedi_probe():

    if (iscsi_host_add(qedi->shost, &pdev->dev)) {
        ...
    }

    rc = qedi_alloc_uio_rings(qedi);
    ...
    rc = qedi_cm_alloc_mem(qedi);

iscsi_host_add() registers the host with sysfs, emitting a uevent. If
userspace responds by issuing a netlink command to create an endpoint,
qedi_ep_connect() assigns the endpoint to qedi->ep_tbl[iscsi_cid].

However, qedi_cm_alloc_mem() allocates qedi->ep_tbl after iscsi_host_add(),
so qedi->ep_tbl would be NULL during this race.


[Severity: High]
This is a pre-existing issue, but is it possible the error path in
__qedi_probe() fails to clean up the LL2 subsystem and kthread?

If an initialization step fails after the hardware packet reception and
receiver kthread are started:

drivers/scsi/qedi/qedi_main.c:__qedi_probe():

    ...
stop_iscsi_func:
    qedi_ops->stop(qedi->cdev);
stop_slowpath:
    qedi_ops->common->slowpath_stop(qedi->cdev);
    ...

The error handler frees the qedi structure but seems to completely omit
stopping the hardware LL2 interface or killing the kthread, which could lead
to a use-after-free when they access the freed qedi structure.


[Severity: High]
This is a pre-existing issue, but would the error path for iscsi_host_add()
failure cause a crash?

drivers/scsi/qedi/qedi_main.c:__qedi_probe():

    if (iscsi_host_add(qedi->shost, &pdev->dev)) {
        QEDI_ERR(&qedi->dbg_ctx,
                 "Could not add iscsi host\n");
        rc = -ENOMEM;
        goto remove_host;
    }

If iscsi_host_add() fails, the code jumps to remove_host and unconditionally
executes iscsi_host_remove(). Since the host was never successfully added to
the driver core, won't this attempt to unregister an uninitialized device?


[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check during
network packet reception in qedi_ll2_rx()?

drivers/scsi/qedi/qedi_main.c:qedi_ll2_rx():

    if (eh->h_proto == htons(ETH_P_8021Q)) {
        memmove((u8 *)eh + VLAN_HLEN, eh, ETH_ALEN * 2);
        eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
        skb_reset_mac_header(skb);
    }

    /* Filter out non FIP/FCoE frames here to free them faster */
    if (eh->h_proto != htons(ETH_P_ARP) && ...

The code reads eh->h_proto without verifying skb->len >= ETH_HLEN. If the
protocol is 802.1Q (VLAN), it invokes skb_pull(skb, VLAN_HLEN).

If a malformed packet is smaller than VLAN_HLEN, skb_pull() returns NULL,
which is then unconditionally dereferenced in the subsequent check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910092405.1300129-1-runyu.xiao@seu.edu.cn?part=1

[PATCH] scsi: qedi: Initialize callback state before registration

[Runyu Xiao]

qedi_get_protocol_tlv_data() can be called asynchronously by QED after
qedi_ops->register_ops(). It takes stats_lock and reads ll2_mtu, but
__qedi_probe() currently registers the callback before initializing the
mutex and assigning the default MTU on the normal probe path.

Initialize the callback-visible state before registering qedi_cb_ops. Keep
the recovery path from reinitializing state because it reuses the existing
qedi context.

Fixes: 3cc5746e5ad7 ("scsi: qedi: Initialize the stats mutex lock")
Cc: stable@vger.kernel.org

Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/scsi/qedi/qedi_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index 227ff7bd1..f394fcbb9 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -2652,8 +2652,6 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
 	snprintf(host_buf, sizeof(host_buf), "host_%d", qedi->shost->host_no);
 	qedi_ops->common->set_name(qedi->cdev, host_buf);
 
-	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
-
 	memset(&params, 0, sizeof(params));
 	params.mtu = DEF_PATH_MTU + IPV6_HDR_LEN + TCP_HDR_LEN;
 	qedi->ll2_mtu = DEF_PATH_MTU;
@@ -2670,6 +2668,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
 		spin_lock_init(&qedi->task_idx_lock);
 		mutex_init(&qedi->stats_lock);
 	}
+	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
 	qedi_ops->ll2->register_cb_ops(qedi->cdev, &qedi_ll2_cb_ops, qedi);
 	qedi_ops->ll2->start(qedi->cdev, &params);
 
-- 
2.34.1

Re: [PATCH] scsi: qedi: Initialize callback state before registration

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of kthread_run() is not checked for errors, resulting in an ERR_PTR being stored and subsequently dereferenced.
- [High] The ll2_recv_thread is leaked on __qedi_probe error paths, causing a task leak and a subsequent use-after-free.
- [High] Missing error check for qedi_ops->ll2->start().
- [High] Memory leak of fastpath arrays and status blocks on __qedi_probe error paths.
- [High] Asynchronous callbacks registered via qedi_ops->register_ops() can execute and schedule uninitialized delayed works, leading to a kernel panic.
- [High] Calling scsi_remove_host() on a host that failed scsi_add_host(), leading to sysfs/device_del warnings or crashes.
- [High] The iSCSI host is exposed to userspace before critical workqueues are allocated, allowing userspace tools to trigger NULL pointer dereferences.
- [High] The driver reads the DMA-updated hardware producer index (sb->pi_array) without a dma_rmb() and without le16_to_cpu().
--

commit 0c41fa3eb18f5cff0e550ef57ea439ee89ae82a1
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: qedi: Initialize callback state before registration

qedi_get_protocol_tlv_data() can be called asynchronously by QED after
qedi_ops->register_ops(). It takes stats_lock and reads ll2_mtu, but
__qedi_probe() currently registers the callback before initializing the
mutex and assigning the default MTU on the normal probe path.

Initialize the callback-visible state before registering qedi_cb_ops. Keep
the recovery path from reinitializing state because it reuses the existing
qedi context.

Fixes: 3cc5746e5ad7 ("scsi: qedi: Initialize the stats mutex lock")
Cc: stable@vger.kernel.org

Assisted-by: LLM Codex
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>

> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index 227ff7bd1bdc9..f394fcbb97e7e 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -2652,8 +2652,6 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
>  	snprintf(host_buf, sizeof(host_buf), "host_%d", qedi->shost->host_no);
>  	qedi_ops->common->set_name(qedi->cdev, host_buf);
>  
> -	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);
> -
>  	memset(&params, 0, sizeof(params));
>  	params.mtu = DEF_PATH_MTU + IPV6_HDR_LEN + TCP_HDR_LEN;
>  	qedi->ll2_mtu = DEF_PATH_MTU;
> @@ -2670,6 +2668,7 @@ static int __qedi_probe(struct pci_dev *pdev, int mode)
>  		spin_lock_init(&qedi->task_idx_lock);
>  		mutex_init(&qedi->stats_lock);
>  	}
> +	qedi_ops->register_ops(qedi->cdev, &qedi_cb_ops, qedi);

[Severity: High]
This is a pre-existing issue, but registering the callbacks here makes them
live before delayed works are initialized.

If an asynchronous hardware event (like a link state change or fan failure)
triggers early, it could call qedi_schedule_hw_err_handler(), which attempts
to schedule the uninitialized qedi->board_disable_work.

The delayed works are initialized much later in __qedi_probe():

	INIT_DELAYED_WORK(&qedi->recovery_work, qedi_recovery_handler);
	INIT_DELAYED_WORK(&qedi->board_disable_work, qedi_board_disable_work);

Could this early registration lead to a kernel panic if a callback fires and
schedules uninitialized work structures?

>  	qedi_ops->ll2->register_cb_ops(qedi->cdev, &qedi_ll2_cb_ops, qedi);
>  	qedi_ops->ll2->start(qedi->cdev, &params);

[Severity: High]
This isn't a bug introduced by this patch, but does this code ignore the
return value from qedi_ops->ll2->start()?

Could ignoring an initialization failure here lead to unpredictable behavior
later?

[Severity: High]
This is a pre-existing issue, but right after this in __qedi_probe(), the
kthread is created:

	qedi->ll2_recv_thread = kthread_run(qedi_ll2_recv_thread, (void *)qedi,
					    "qedi_ll2_thread");

If kthread_run() fails due to memory pressure, it returns an ERR_PTR.
Does this code assign the ERR_PTR without using IS_ERR() to check for
failure?

If so, later in teardown or RX paths, if (qedi->ll2_recv_thread) will
evaluate to true, and passing the ERR_PTR to kthread_stop() in
__qedi_remove() or wake_up_process() in qedi_ll2_rx() could cause a panic.

[Severity: High]
This is a pre-existing issue, but if __qedi_probe() fails after successfully
creating qedi->ll2_recv_thread, it jumps to error paths:

	rc = qedi_ops->start(qedi->cdev, &qedi->tasks, qedi,
			     qedi_iscsi_event_cb);
	if (rc) {
		...
		goto stop_slowpath;
	}

The error path frees the qedi struct via iscsi_host_free() without stopping
the background thread. Does this leak the thread and result in a
use-after-free when the orphaned thread continues to access qedi?

[Severity: High]
This is a pre-existing issue, but when iscsi_host_add() fails in
__qedi_probe(), the code jumps to remove_host:

	if (iscsi_host_add(qedi->shost, &pdev->dev)) {
		rc = -ENOMEM;
		goto remove_host;
	}

The remove_host label unconditionally calls iscsi_host_remove(), which wraps
scsi_remove_host(). Can calling scsi_remove_host() on a host that failed
scsi_add_host() lead to sysfs or kobject core warnings and crashes?

[Severity: High]
This is a pre-existing issue, but iscsi_host_add() is called and exposes the
device to userspace before critical workqueues are allocated:

	if (iscsi_host_add(qedi->shost, &pdev->dev)) {
		...

And later:

	qedi->offload_thread = alloc_workqueue("qedi_ofld%d", ...);

Could userspace immediately respond via netlink to create an endpoint,
invoking qedi_ep_connect() which calls queue_work(qedi->offload_thread, ...),
resulting in a NULL pointer dereference because the workqueue isn't allocated
yet?

[Severity: High]
This isn't a bug introduced by this patch, but memory and DMA blocks are
allocated in qedi_prepare_fp() earlier in __qedi_probe():

	rc = qedi_prepare_fp(qedi);
	if (rc) { ... }

If a subsequent initialization step fails, the error path does not call
qedi_destroy_fp(qedi). Does this code leak the fastpath arrays and status
blocks on error paths?

[Severity: High]
This is a pre-existing issue, but the driver reads the DMA-updated hardware
producer index without a memory barrier or endianness conversion:

drivers/scsi/qedi/qedi_main.c:qedi_process_completions() {
	...
	prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
	...
}

Does reading sb->pi_array (an array of __le16 in DMA memory) directly into a
native u16 without le16_to_cpu(), and without a subsequent dma_rmb() before
fetching ring entries, lead to reading stale data on relaxed memory models or
broken functionality on big-endian architectures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910112947.1350877-1-runyu.xiao@seu.edu.cn?part=1