Hi Dan,
On Tue, 2013-09-10 at 15:21 +0300, Dan Carpenter wrote:
> Hello Nicholas Bellinger,
>
> The patch 2450acf26519: "target: Add support for COMPARE_AND_WRITE
> emulation" from Aug 19, 2013, leads to the following static checker
> warning: "drivers/target/target_core_sbc.c:508 sbc_compare_and_write()
> warn: 'sem:&dev->caw_sem' is sometimes locked here and
> sometimes unlocked."
>
> drivers/target/target_core_sbc.c
> 502 /*
> 503 * Submit the READ first for COMPARE_AND_WRITE to perform the
> 504 * comparision using SGLs at cmd->t_bidi_data_sg..
> 505 */
> 506 rc = down_interruptible(&dev->caw_sem);
> 507 if ((rc != 0) || signal_pending(current))
> 508 return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> When we return here, we don't know if we are holding the lock or not.
> I believe that we release this lock in compare_and_write_callback() so
> this is actually a double unlock bug and not a deadlock.
Mmmmm, your absolutely correct here.
This failure needs to explicitly clear cmd->transport_complete_callback
in order to avoid compare_and_write_callback() -> double unlock of
->caw_sem upon miscompare.
>
> 509
> 510 ret = cmd->execute_rw(cmd, cmd->t_bidi_data_sg, cmd->t_bidi_data_nents,
> 511 DMA_FROM_DEVICE);
> 512 if (ret) {
> 513 up(&dev->caw_sem);
> ^^^^^^^^^^^^^^^^^^
> This may be a double unlock bug as well.
>
<nod>, ditto here as well.
> 514 return ret;
> 515 }
> 516 /*
> 517 * Unlock of dev->caw_sem to occur in compare_and_write_callback()
> 518 * upon MISCOMPARE, or in compare_and_write_done() upon completion
> 519 * of WRITE instance user-data.
> 520 */
> 521 return TCM_NO_SENSE;
>
>
OK, here's an incremental patch to clear the callback for these two
failure cases, and also explicitly check for the callback in
transport_generic_request_failure().
Thank you,
--nab
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 2572ba7..6c17295 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -504,12 +504,15 @@ sbc_compare_and_write(struct se_cmd *cmd)
* comparision using SGLs at cmd->t_bidi_data_sg..
*/
rc = down_interruptible(&dev->caw_sem);
- if ((rc != 0) || signal_pending(current))
+ if ((rc != 0) || signal_pending(current)) {
+ cmd->transport_complete_callback = NULL;
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+ }
ret = cmd->execute_rw(cmd, cmd->t_bidi_data_sg, cmd->t_bidi_data_nents,
DMA_FROM_DEVICE);
if (ret) {
+ cmd->transport_complete_callback = NULL;
up(&dev->caw_sem);
return ret;
}
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 2cc723d..4aace78 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1563,7 +1563,8 @@ void transport_generic_request_failure(struct se_cmd *cmd,
* Handle special case for COMPARE_AND_WRITE failure, where the
* callback is expected to drop the per device ->caw_mutex.
*/
- if (cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE)
+ if ((cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE) &&
+ cmd->transport_complete_callback)
cmd->transport_complete_callback(cmd);
switch (sense_reason) {
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
