On 11/21/22 2:02 PM, Mike Christie wrote:
> On 11/21/22 3:27 AM, Maurizio Lombardi wrote:
>> While handling an I/O completion for the compare portion of a
>> COMPARE_AND_WRITE command, it may happen that the
>> compare_and_write_callback function submits new bio structs
>> while still in softirq context.
>>
>> low level drivers like md raid5 do not expect their make_request
>> call to be used in softirq context, they call into schedule() and
>> create a deadlocked system.
>>
>> __schedule at ffffffff873a0807
>> schedule at ffffffff873a0cc5
>> raid5_get_active_stripe at ffffffffc0875744 [raid456]
>> raid5_make_request at ffffffffc0875a50 [raid456]
>> md_handle_request at ffffffff8713b9f9
>> md_make_request at ffffffff8713bacb
>> generic_make_request at ffffffff86e6f14b
>> submit_bio at ffffffff86e6f27c
>> iblock_submit_bios at ffffffffc0b4e4dc [target_core_iblock]
>> iblock_execute_rw at ffffffffc0b4f3ce [target_core_iblock]
>> __target_execute_cmd at ffffffffc1090079 [target_core_mod]
>> compare_and_write_callback at ffffffffc1093602 [target_core_mod]
>> target_cmd_interrupted at ffffffffc108d1ec [target_core_mod]
>> target_complete_cmd_with_sense at ffffffffc108d27c [target_core_mod]
>> iblock_complete_cmd at ffffffffc0b4e23a [target_core_iblock]
>> dm_io_dec_pending at ffffffffc00db29e [dm_mod]
>> clone_endio at ffffffffc00dbf07 [dm_mod]
>> raid5_align_endio at ffffffffc086d6c2 [raid456]
>> blk_update_request at ffffffff86e6d950
>> scsi_end_request at ffffffff87063d48
>> scsi_io_completion at ffffffff87063ee8
>> blk_complete_reqs at ffffffff86e77b05
>> __softirqentry_text_start at ffffffff876000d7
>>
>> This problem appears to be an issue between target_cmd_interrupted()
>> and compare_and_write_callback(). target_cmd_interrupted() calls the
>> se_cmd's transport_complete_callback function pointer if the se_cmd
>> is being stopped or aborted, and CMD_T_ABORTED was set on the se_cmd.
>>
>> When calling compare_and_write_callback(), the success parameter
>> was set to false. target_cmd_interrupted() seems to expect this
>> means the callback will do cleanup that does not require a process
>> context. But compare_and_write_callback() ignores the parameter if
>> there was I/O done for the compare part of COMPARE_AND_WRITE.
>>
>> Since there was data, the function continued on, passed the compare,
>> and issued a write while ignoring the value of the success parameter.
>> The submit of a bio for the write portion of the COMPARE_AND_WRITE
>> then causes schedule to be unsafely called from the softirq context.
>>
>> Fix the bug in compare_and_write_callback by jumping
>> to the out label if success == "false", after checking if
>> we have been called by transport_generic_request_failure();
>> The command is being aborted or stopped so there is no
>> need to submit the write bio for the write part
>> of the COMPARE_AND_WRITE command.
>>
>> Signed-off-by: Maurizio Lombardi <mlombard@xxxxxxxxxx>
>> ---
>> drivers/target/target_core_sbc.c | 22 ++++++++++++++++------
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
>> index 1e3216de1e04..80d7a4419c4c 100644
>> --- a/drivers/target/target_core_sbc.c
>> +++ b/drivers/target/target_core_sbc.c
>> @@ -454,12 +454,22 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
>> sense_reason_t ret = TCM_NO_SENSE;
>> int i;
>>
>> - /*
>> - * Handle early failure in transport_generic_request_failure(),
>> - * which will not have taken ->caw_sem yet..
>> - */
>> - if (!success && (!cmd->t_data_sg || !cmd->t_bidi_data_sg))
>> - return TCM_NO_SENSE;
>> + if (!success) {
>> + /*
>> + * Handle early failure in transport_generic_request_failure(),
>> + * which will not have taken ->caw_sem yet..
>> + */
>> + if (!cmd->t_data_sg || !cmd->t_bidi_data_sg)
>> + return TCM_NO_SENSE;
>> +
>> + /*
>> + * The command has been stopped or aborted so
>> + * we don't have to perform the write operation.
>> + */
>> + WARN_ON(!(cmd->transport_state &
>> + (CMD_T_ABORTED | CMD_T_STOP)));
>> + goto out;
>> + }
>
> Instead of having the "bool success" arg then the callback figuring out the
> context/state it's being called from, would it be nicer to have the caller
> tell us. Change the bool to a:
>
> enum target_callback_state {
> TARGET_CALLBACK_STATE_SUCCESS,
> TARGET_CALLBACK_STATE_ABORTED,
> TARGET_CALLBACK_STATE_FAILED,
> };
>
Was thinking about this more. That might not be as nice as I thought, because we
have to determine when it fails vs fails early. We only have the one user now, so
it might be overkill to change all the callers.
Your patch seems ok.
Reviewed-by: Mike Christie <michael.christie@xxxxxxxxxx>
