[PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Bart Van Assche]

There is a TOCTOU race in ufshcd_compl_one_cqe(): hba->dev_cmd.complete
may be cleared from another thread after it has been checked and before
it is used. Fix this race by moving the device command completion from
the stack of the device command submitter into struct ufs_hba. This
patch fixes the following kernel crash:

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008
Call trace:
 _raw_spin_lock_irqsave+0x34/0x80
 complete+0x24/0xb8
 ufshcd_compl_one_cqe+0x13c/0x4f0
 ufshcd_mcq_poll_cqe_lock+0xb4/0x108
 ufshcd_intr+0x2f4/0x444
 __handle_irq_event_percpu+0xbc/0x250
 handle_irq_event+0x48/0xb0

Fixes: 5a0b0cb9bee7 ("[SCSI] ufs: Add support for sending NOP OUT UPIU")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared to v1:
 - Call init_completion() once instead of every time a device management
   command is submitted.

 drivers/ufs/core/ufshcd.c | 25 ++++++-------------------
 include/ufs/ufshcd.h      |  2 +-
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4e1e214fc5a2..3288a7da73dc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3176,16 +3176,10 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 	int err;
 
 retry:
-	time_left = wait_for_completion_timeout(hba->dev_cmd.complete,
+	time_left = wait_for_completion_timeout(&hba->dev_cmd.complete,
 						time_left);
 
 	if (likely(time_left)) {
-		/*
-		 * The completion handler called complete() and the caller of
-		 * this function still owns the @lrbp tag so the code below does
-		 * not trigger any race conditions.
-		 */
-		hba->dev_cmd.complete = NULL;
 		err = ufshcd_get_tr_ocs(lrbp, NULL);
 		if (!err)
 			err = ufshcd_dev_cmd_completion(hba, lrbp);
@@ -3199,7 +3193,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 			/* successfully cleared the command, retry if needed */
 			if (ufshcd_clear_cmd(hba, lrbp->task_tag) == 0)
 				err = -EAGAIN;
-			hba->dev_cmd.complete = NULL;
 			return err;
 		}
 
@@ -3215,11 +3208,9 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 			spin_lock_irqsave(&hba->outstanding_lock, flags);
 			pending = test_bit(lrbp->task_tag,
 					   &hba->outstanding_reqs);
-			if (pending) {
-				hba->dev_cmd.complete = NULL;
+			if (pending)
 				__clear_bit(lrbp->task_tag,
 					    &hba->outstanding_reqs);
-			}
 			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 
 			if (!pending) {
@@ -3237,8 +3228,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 			spin_lock_irqsave(&hba->outstanding_lock, flags);
 			pending = test_bit(lrbp->task_tag,
 					   &hba->outstanding_reqs);
-			if (pending)
-				hba->dev_cmd.complete = NULL;
 			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
 
 			if (!pending) {
@@ -3272,13 +3261,9 @@ static void ufshcd_dev_man_unlock(struct ufs_hba *hba)
 static int ufshcd_issue_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
 			  const u32 tag, int timeout)
 {
-	DECLARE_COMPLETION_ONSTACK(wait);
 	int err;
 
-	hba->dev_cmd.complete = &wait;
-
 	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
-
 	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
 	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
 
@@ -5585,12 +5570,12 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 		ufshcd_release_scsi_cmd(hba, lrbp);
 		/* Do not touch lrbp after scsi done */
 		scsi_done(cmd);
-	} else if (hba->dev_cmd.complete) {
+	} else {
 		if (cqe) {
 			ocs = le32_to_cpu(cqe->status) & MASK_OCS;
 			lrbp->utr_descriptor_ptr->header.ocs = ocs;
 		}
-		complete(hba->dev_cmd.complete);
+		complete(&hba->dev_cmd.complete);
 	}
 }
 
@@ -10475,6 +10460,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	spin_lock_init(&hba->clk_gating.lock);
 
+	init_completion(&hba->dev_cmd.complete);
+
 	err = ufshcd_hba_init(hba);
 	if (err)
 		goto out_error;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index e3909cc691b2..f56050ce9445 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -246,7 +246,7 @@ struct ufs_query {
 struct ufs_dev_cmd {
 	enum dev_cmd_type type;
 	struct mutex lock;
-	struct completion *complete;
+	struct completion complete;
 	struct ufs_query query;
 };
 

RE: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Avri Altman]

> 
> There is a TOCTOU race in ufshcd_compl_one_cqe(): hba->dev_cmd.complete
> may be cleared from another thread after it has been checked and before it is
> used. Fix this race by moving the device command completion from the stack
> of the device command submitter into struct ufs_hba. This patch fixes the
> following kernel crash:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000008 Call trace:
>  _raw_spin_lock_irqsave+0x34/0x80
>  complete+0x24/0xb8
>  ufshcd_compl_one_cqe+0x13c/0x4f0
>  ufshcd_mcq_poll_cqe_lock+0xb4/0x108
>  ufshcd_intr+0x2f4/0x444
>  __handle_irq_event_percpu+0xbc/0x250
>  handle_irq_event+0x48/0xb0
> 
> Fixes: 5a0b0cb9bee7 ("[SCSI] ufs: Add support for sending NOP OUT UPIU")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> Changes compared to v1:
>  - Call init_completion() once instead of every time a device management
>    command is submitted.
Shouldn't you now call for reinit_completion now?
before wait_for_dev? Or at ufshcd_dev_cmd_completion ?

Thanks,
Avri

> 
>  drivers/ufs/core/ufshcd.c | 25 ++++++-------------------
>  include/ufs/ufshcd.h      |  2 +-
>  2 files changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 4e1e214fc5a2..3288a7da73dc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -3176,16 +3176,10 @@ static int ufshcd_wait_for_dev_cmd(struct
> ufs_hba *hba,
>  	int err;
> 
>  retry:
> -	time_left = wait_for_completion_timeout(hba->dev_cmd.complete,
> +	time_left = wait_for_completion_timeout(&hba->dev_cmd.complete,
>  						time_left);
> 
>  	if (likely(time_left)) {
> -		/*
> -		 * The completion handler called complete() and the caller of
> -		 * this function still owns the @lrbp tag so the code below
> does
> -		 * not trigger any race conditions.
> -		 */
> -		hba->dev_cmd.complete = NULL;
>  		err = ufshcd_get_tr_ocs(lrbp, NULL);
>  		if (!err)
>  			err = ufshcd_dev_cmd_completion(hba, lrbp); @@ -
> 3199,7 +3193,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>  			/* successfully cleared the command, retry if needed
> */
>  			if (ufshcd_clear_cmd(hba, lrbp->task_tag) == 0)
>  				err = -EAGAIN;
> -			hba->dev_cmd.complete = NULL;
>  			return err;
>  		}
> 
> @@ -3215,11 +3208,9 @@ static int ufshcd_wait_for_dev_cmd(struct
> ufs_hba *hba,
>  			spin_lock_irqsave(&hba->outstanding_lock, flags);
>  			pending = test_bit(lrbp->task_tag,
>  					   &hba->outstanding_reqs);
> -			if (pending) {
> -				hba->dev_cmd.complete = NULL;
> +			if (pending)
>  				__clear_bit(lrbp->task_tag,
>  					    &hba->outstanding_reqs);
> -			}
>  			spin_unlock_irqrestore(&hba->outstanding_lock,
> flags);
> 
>  			if (!pending) {
> @@ -3237,8 +3228,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>  			spin_lock_irqsave(&hba->outstanding_lock, flags);
>  			pending = test_bit(lrbp->task_tag,
>  					   &hba->outstanding_reqs);
> -			if (pending)
> -				hba->dev_cmd.complete = NULL;
>  			spin_unlock_irqrestore(&hba->outstanding_lock,
> flags);
> 
>  			if (!pending) {
> @@ -3272,13 +3261,9 @@ static void ufshcd_dev_man_unlock(struct
> ufs_hba *hba)  static int ufshcd_issue_dev_cmd(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp,
>  			  const u32 tag, int timeout)
>  {
> -	DECLARE_COMPLETION_ONSTACK(wait);
>  	int err;
> 
> -	hba->dev_cmd.complete = &wait;
> -
>  	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp-
> >ucd_req_ptr);
> -
>  	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
>  	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
> 
> @@ -5585,12 +5570,12 @@ void ufshcd_compl_one_cqe(struct ufs_hba
> *hba, int task_tag,
>  		ufshcd_release_scsi_cmd(hba, lrbp);
>  		/* Do not touch lrbp after scsi done */
>  		scsi_done(cmd);
> -	} else if (hba->dev_cmd.complete) {
> +	} else {
>  		if (cqe) {
>  			ocs = le32_to_cpu(cqe->status) & MASK_OCS;
>  			lrbp->utr_descriptor_ptr->header.ocs = ocs;
>  		}
> -		complete(hba->dev_cmd.complete);
> +		complete(&hba->dev_cmd.complete);
>  	}
>  }
> 
> @@ -10475,6 +10460,8 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem *mmio_base, unsigned int irq)
>  	 */
>  	spin_lock_init(&hba->clk_gating.lock);
> 
> +	init_completion(&hba->dev_cmd.complete);
> +
>  	err = ufshcd_hba_init(hba);
>  	if (err)
>  		goto out_error;
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
> e3909cc691b2..f56050ce9445 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -246,7 +246,7 @@ struct ufs_query {
>  struct ufs_dev_cmd {
>  	enum dev_cmd_type type;
>  	struct mutex lock;
> -	struct completion *complete;
> +	struct completion complete;
>  	struct ufs_query query;
>  };
> 

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Bart Van Assche]

On 3/15/25 1:46 AM, Avri Altman wrote:
> Shouldn't you now call for reinit_completion now?
> before wait_for_dev? Or at ufshcd_dev_cmd_completion ?

complete() increments the counter in struct completion and
wait_for_complete() decrements it, isn't it? From
kernel/sched/completion.c:

void complete(struct completion *x)
{
	complete_with_flags(x, 0);
}
EXPORT_SYMBOL(complete);

static void complete_with_flags(struct completion *x, int wake_flags)
{
	unsigned long flags;

	raw_spin_lock_irqsave(&x->wait.lock, flags);

	if (x->done != UINT_MAX)
		x->done++;
	swake_up_locked(&x->wait, wake_flags);
	raw_spin_unlock_irqrestore(&x->wait.lock, flags);
}

As one can see complete() increments x->done if it is less than
UINT_MAX, which should be the case in the UFS driver.


 From the same file:

void __sched wait_for_completion(struct completion *x)
{
	wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE);
}
EXPORT_SYMBOL(wait_for_completion);

static long __sched
wait_for_common(struct completion *x, long timeout, int state)
{
	return __wait_for_common(x, schedule_timeout, timeout, state);
}

static inline long __sched
__wait_for_common(struct completion *x,
		  long (*action)(long), long timeout, int state)
{
	might_sleep();

	complete_acquire(x);

	raw_spin_lock_irq(&x->wait.lock);
	timeout = do_wait_for_common(x, action, timeout, state);
	raw_spin_unlock_irq(&x->wait.lock);

	complete_release(x);

	return timeout;
}

static inline long __sched
do_wait_for_common(struct completion *x,
		   long (*action)(long), long timeout, int state)
{
	if (!x->done) {
		DECLARE_SWAITQUEUE(wait);

		do {
			if (signal_pending_state(state, current)) {
				timeout = -ERESTARTSYS;
				break;
			}
			__prepare_to_swait(&x->wait, &wait);
			__set_current_state(state);
			raw_spin_unlock_irq(&x->wait.lock);
			timeout = action(timeout);
			raw_spin_lock_irq(&x->wait.lock);
		} while (!x->done && timeout);
		__finish_swait(&x->wait, &wait);
		if (!x->done)
			return timeout;
	}
	if (x->done != UINT_MAX)
		x->done--;
	return timeout ?: 1;
}

If I read the above code correctly, it waits until x->done != 0 or the
timeout has been reached. x->done is decremented if a strictly positive
value is returned. do_wait_for_common() ignores pending signals because
state == TASK_UNINTERRUPTIBLE.

Bart.

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Peter Wang (王信友)]

On Fri, 2025-03-14 at 15:51 -0700, Bart Van Assche wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> There is a TOCTOU race in ufshcd_compl_one_cqe(): hba-
> >dev_cmd.complete
> may be cleared from another thread after it has been checked and
> before
> it is used. Fix this race by moving the device command completion
> from
> the stack of the device command submitter into struct ufs_hba. This
> patch fixes the following kernel crash:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000008
> Call trace:
>  _raw_spin_lock_irqsave+0x34/0x80
>  complete+0x24/0xb8
>  ufshcd_compl_one_cqe+0x13c/0x4f0
>  ufshcd_mcq_poll_cqe_lock+0xb4/0x108
>  ufshcd_intr+0x2f4/0x444
>  __handle_irq_event_percpu+0xbc/0x250
>  handle_irq_event+0x48/0xb0
> 
> Fixes: 5a0b0cb9bee7 ("[SCSI] ufs: Add support for sending NOP OUT
> UPIU")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> Changes compared to v1:
>  - Call init_completion() once instead of every time a device
> management
>    command is submitted.
> 

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Martin K. Petersen]

Bart,

> There is a TOCTOU race in ufshcd_compl_one_cqe():
> hba->dev_cmd.complete may be cleared from another thread after it has
> been checked and before it is used. Fix this race by moving the device
> command completion from the stack of the device command submitter into
> struct ufs_hba. This patch fixes the following kernel crash:

Applied to 6.15/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Bart Van Assche]

On 3/20/25 5:48 PM, Martin K. Petersen wrote:
> Applied to 6.15/scsi-staging, thanks!

Hi Martin,

Has a pull request already been sent to Linus for the changes on the
6.15/scsi-staging branch?

Thanks,

Bart.

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Martin K. Petersen]

Hi Bart!

> Has a pull request already been sent to Linus for the changes on the
> 6.15/scsi-staging branch?

scsi-staging is just for seeing what breaks. Patches aren't considered
submission material until they show up in either scsi-queue or
scsi-fixes.

However, commit 20b97acc4caf ("scsi: ufs: core: Fix a race condition
related to device commands") is in scsi-queue and thus technically ready
for submission.

Is there a problem with this patch?

If I were to drop it, I'd have to redo a few things in staging. I can do
that, but at this stage I'd prefer an incremental patch.

-- 
Martin K. Petersen

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Bart Van Assche]

On 4/7/25 11:55 AM, Martin K. Petersen wrote:
> Is there a problem with this patch?

Hi Martin,

That patch is fine as far as I know. My question is about timing. Is my
understanding correct that all these patches will be merged during the
6.16 merge window (three months from now)?

Thanks,

Bart.

$ git log --format=oneline origin/master..mkp-scsi/for-next
1b4902f0a4f20aaea14d51a378368fa697467901 scsi: megaraid_sas: Driver 
version update to 07.734.00.00-rc1
aad9945623ab4029ae7789609fb6166c97976c62 scsi: megaraid_sas: Block 
zero-length ATA VPD inquiry
a63b69f05f999acae91b0b50d7c5fe4fb241dbaf scsi: scsi_transport_srp: 
Replace min/max nesting with clamp()
1fd2e77b889761d9bde0c580518689d1d8e83117 scsi: ufs: core: Add device 
level exception support
bdab40480b146e2f37f4c7164cb47f526e77ee6d scsi: ufs: core: Rename 
ufshcd_wb_presrv_usrspc_keep_vcc_on()
a2d5a0072235a69749ceb04c1a26dc75df66a31a scsi: smartpqi: Use 
is_kdump_kernel() to check for kdump
f7b705c238d1483f0a766e2b20010f176e5c0fb7 scsi: pm80xx: Set phy_attached 
to zero when device is gone
8a65b75dc4b235349fa6f3c89d381405956d431f Merge patch series "ufs-exynos 
stability fixes for gs101"
cd4c0025069f16fc666c6ffc56c49c9b1154841f scsi: ufs: exynos: gs101: Put 
UFS device in reset on .suspend()
67e4085015c33bf2fb552af1f171c58b81ef0616 scsi: ufs: exynos: Move phy 
calls to .exit() callback
deac9ad496ec17e1ec06848964ecc635bdaca703 scsi: ufs: exynos: Enable PRDT 
pre-fetching with UFSHCD_CAP_CRYPTO
7f05fd9a3b6fb3a9abc5a748307d11831c03175f scsi: ufs: exynos: Ensure 
consistent phy reference counts
f92bb7436802f8eb7ee72dc911a33c8897fde366 scsi: ufs: exynos: Disable iocc 
if dma-coherent property isn't set
68f5ef7eebf0f41df4d38ea55a54c2462af1e3d6 scsi: ufs: exynos: Move UFS 
shareability value to drvdata
3d101165e72316775947d71321d97194f03dfef3 scsi: ufs: exynos: Ensure 
pre_link() executes before exynos_ufs_phy_init()
72eea84a1092b50a10eeecfeba4b28ac9f1312ab scsi: iscsi: Fix missing 
scsi_host_put() in error path
20b97acc4cafa2be8ac91a777de135110e58a90b scsi: ufs: core: Fix a race 
condition related to device commands
daff37f00c7506ca322ccfce95d342022f06ec58 scsi: hisi_sas: Fix I/O errors 
caused by hardware port ID changes
8aa580cd92843b60d4d6331f3b0a9e8409bb70eb scsi: hisi_sas: Enable force 
phy when SATA disk directly connected

Re: [PATCH v2] scsi: ufs: core: Fix a race condition related to device commands

[Martin K. Petersen]

Bart,

> That patch is fine as far as I know. My question is about timing. Is
> my understanding correct that all these patches will be merged during
> the 6.16 merge window (three months from now)?

The patches currently merged will be pulled into 6.15/scsi-fixes once I
set up the new trees. There is almost always an outstanding delta at the
beginning of each cycle since I don't stop applying fixes during the
merge window.

-- 
Martin K. Petersen