linux-kernel - Re: [PATCH] scsi: sd: add runtime pm to open / release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Jun 2020 12:15:36 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Martin Kepplinger <martin.kepplinger@...i.sm>
Cc:     Bart Van Assche <bvanassche@....org>, jejb@...ux.ibm.com,
        Can Guo <cang@...eaurora.org>, martin.petersen@...cle.com,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...i.sm
Subject: Re: [PATCH] scsi: sd: add runtime pm to open / release

On Mon, Jun 29, 2020 at 11:42:59AM +0200, Martin Kepplinger wrote:
> 
> 
> On 26.06.20 17:44, Alan Stern wrote:
> > Martin's best approach would be to add some debugging code to find out why 
> > blk_queue_enter() isn't calling bkl_pm_request_resume(), or why that call 
> > doesn't lead to pm_request_resume().
> > 
> > Alan Stern
> > 
> 
> Hi Alan,
> 
> blk_queue_enter() always - especially when sd is runtime suspended and I
> try to mount as above - sets success to be true for me, so never
> continues down to bkl_pm_request_resume(). All I see is "PM: Removing
> info for No Bus:sda1".

Aha.  Looking at this more closely, it's apparent that the code in 
blk-core.c contains a logic bug: It assumes that if the BLK_MQ_REQ_PREEMPT 
flag is set then the request can be issued regardless of the queue's 
runtime status.  That is not correct when the queue is suspended.

Below is my attempt to fix this up.  I'm not sure that the patch is 
entirely correct, but it should fix this logic bug.  I would appreciate a 
critical review.

Martin, does this fix the problem?

Alan Stern



Index: usb-devel/block/blk-core.c
===================================================================
--- usb-devel.orig/block/blk-core.c
+++ usb-devel/block/blk-core.c
@@ -423,7 +423,8 @@ int blk_queue_enter(struct request_queue
 			 * responsible for ensuring that that counter is
 			 * globally visible before the queue is unfrozen.
 			 */
-			if (pm || !blk_queue_pm_only(q)) {
+			if ((pm && q->rpm_status != RPM_SUSPENDED) ||
+			    !blk_queue_pm_only(q)) {
 				success = true;
 			} else {
 				percpu_ref_put(&q->q_usage_counter);
@@ -448,8 +449,7 @@ int blk_queue_enter(struct request_queue
 
 		wait_event(q->mq_freeze_wq,
 			   (!q->mq_freeze_depth &&
-			    (pm || (blk_pm_request_resume(q),
-				    !blk_queue_pm_only(q)))) ||
+			    blk_pm_resume_queue(pm, q)) ||
 			   blk_queue_dying(q));
 		if (blk_queue_dying(q))
 			return -ENODEV;
Index: usb-devel/block/blk-pm.h
===================================================================
--- usb-devel.orig/block/blk-pm.h
+++ usb-devel/block/blk-pm.h
@@ -6,11 +6,14 @@
 #include <linux/pm_runtime.h>
 
 #ifdef CONFIG_PM
-static inline void blk_pm_request_resume(struct request_queue *q)
+static inline int blk_pm_resume_queue(const bool pm, struct request_queue *q)
 {
-	if (q->dev && (q->rpm_status == RPM_SUSPENDED ||
-		       q->rpm_status == RPM_SUSPENDING))
-		pm_request_resume(q->dev);
+	if (!q->dev || !blk_queue_pm_only(q))
+		return 1;	/* Nothing to do */
+	if (pm && q->rpm_status != RPM_SUSPENDED)
+		return 1;	/* Request allowed */
+	pm_request_resume(q->dev);
+	return 0;
 }
 
 static inline void blk_pm_mark_last_busy(struct request *rq)
@@ -44,8 +47,9 @@ static inline void blk_pm_put_request(st
 		--rq->q->nr_pending;
 }
 #else
-static inline void blk_pm_request_resume(struct request_queue *q)
+static inline int blk_pm_resume_queue(const bool pm, struct request_queue *q)
 {
+	return 1;
 }
 
 static inline void blk_pm_mark_last_busy(struct request *rq)