Skip to contentSkip to conversation

zfcp: spin_lock_irqsave() is not nestable

PATCH Applied Dan Carpenter 4 participants 6 messages Oct 13, 2016 – Oct 25, 2016

Subsystems (4)
SCSI Kernel janitors s390 s390 zfcp driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 zfcp: spin_lock_irqsave() is not nestable

We accidentally overwrite the original saved value of "flags" so that we can't re-enable IRQs at the end of the function. Presumably this function is mostly called with IRQs disabled or it would be obvious in testing.

Fixes aceeffbb59bb ("zfcp: trace full payload of all SAN records (req,resp,iels)")
Signed-off-by Dan Carpenter <[email protected]>
drivers/s390/scsi/zfcp_dbf.c +1 -1
index 637cf89..5810019 100644
@@ -384,7 +384,7 @@ void zfcp_dbf_san(char *tag, struct zfcp_dbf *dbf,
 	/* if (len > rec_len):
 	 * dump data up to cap_len ignoring small duplicate in rec->payload
 	 */
-	spin_lock_irqsave(&dbf->pay_lock, flags);
+	spin_lock(&dbf->pay_lock);
 	memset(payload, 0, sizeof(*payload));
 	memcpy(payload->area, paytag, ZFCP_DBF_TAG_LEN);
 	payload->fsf_req_id = req_id;
#1 lore raw

Dan, many thanks for catching this! Sparse did not notice, is there other tooling that would find such things?

James, Martin, could you please queue this as fix for one of my patches that went into the 4.9 merge window, so for 4.9-rc I guess?
https://lkml.kernel.org/r/20161013085358.GH16198@mwanda
or
https://lkml.org/lkml/2016/10/13/94

Quoted 6 lines · Dan Carpenter

We accidentally overwrite the original saved value of "flags" so that we can't re-enable IRQs at the end of the function. Presumably this function is mostly called with IRQs disabled or it would be obvious in testing.

Fixes aceeffbb59bb ("zfcp: trace full payload of all SAN records (req,resp,iels)")
Cc <[email protected]> #2.6.38+
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>
Signed-off-by Steffen Maier <[email protected]>
Quoted 15 lines
drivers/s390/scsi/zfcp_dbf.c +1 -1
index 637cf89..5810019 100644
@@ -384,7 +384,7 @@ void zfcp_dbf_san(char *tag, struct zfcp_dbf *dbf,
 	/* if (len > rec_len):
 	 * dump data up to cap_len ignoring small duplicate in rec->payload
 	 */
-	spin_lock_irqsave(&dbf->pay_lock, flags);
+	spin_lock(&dbf->pay_lock);
 	memset(payload, 0, sizeof(*payload));
 	memcpy(payload->area, paytag, ZFCP_DBF_TAG_LEN);
 	payload->fsf_req_id = req_id;
Signature · 15 lines
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#2 lore raw
Quoted 2 lines · Steffen Maier

Dan, many thanks for catching this! Sparse did not notice, is there other tooling that would find such things?

This was a Smatch warning.

regards,
dan carpenter

Applied
#3 lore raw
In reply to Steffen Maier
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Steffen" = Steffen Maier <[email protected]> writes:

Steffen> could you please queue this as fix for one of my patches that Steffen> went into the 4.9 merge window, so for 4.9-rc I guess?

Applied to 4.9/scsi-fixes.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
#4 lore raw
Quoted 7 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Steffen" = Steffen Maier <[email protected]> writes:

Steffen> could you please queue this as fix for one of my patches that Steffen> went into the 4.9 merge window, so for 4.9-rc I guess?

Applied to 4.9/scsi-fixes.

FWIW, I do see rcu stall errors with 4.9-rc1 from time to time, so I assume that this fix is not only theoretical but fixes a real life issue.

Tested-by
#5 lore raw
Quoted 11 lines · Christian Borntraeger
Quoted 7 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Steffen" = Steffen Maier <[email protected]> writes:

Steffen> could you please queue this as fix for one of my patches that Steffen> went into the 4.9 merge window, so for 4.9-rc I guess?

Applied to 4.9/scsi-fixes.

FWIW, I do see rcu stall errors with 4.9-rc1 from time to time, so I assume that this fix is not only theoretical but fixes a real life issue.

Yes, with that patch the rcu stalls are gone, I assume its already on a branch that does not rebase, otherwise feel free to add

Tested-by Christian Borntraeger <[email protected]>