[PATCH] target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()

[PATCH] target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()

[Bart Van Assche]

Fixes: aa73237dcb2d ("scsi: target/core: Always call transport_complete_callback() upon failure")
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Hi Martin,

This patch fixes a bug that was introduced during the merge window. Please consider
this patch for a v4.20-rc<n> kernel.

 drivers/target/target_core_sbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 1ac1f7d2e6c9..7c8433e32906 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -480,7 +480,8 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 	if (cmd->scsi_status) {
 		pr_debug("compare_and_write_callback: non zero scsi_status:"
 			" 0x%02x\n", cmd->scsi_status);
-		*post_ret = 1;
+		if (post_ret)
+			*post_ret = 1;
 		if (cmd->scsi_status = SAM_STAT_CHECK_CONDITION)
 			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		goto out;
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH] target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()

[David Disseldorp]

Hi Bart,

On Mon,  5 Nov 2018 09:23:17 -0800, Bart Van Assche wrote:

> Fixes: aa73237dcb2d ("scsi: target/core: Always call transport_complete_callback() upon failure")
...
> This patch fixes a bug that was introduced during the merge window. Please consider
> this patch for a v4.20-rc<n> kernel.
> 
>  drivers/target/target_core_sbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index 1ac1f7d2e6c9..7c8433e32906 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -480,7 +480,8 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
>  	if (cmd->scsi_status) {
>  		pr_debug("compare_and_write_callback: non zero scsi_status:"
>  			" 0x%02x\n", cmd->scsi_status);
> -		*post_ret = 1;
> +		if (post_ret)
> +			*post_ret = 1;
>  		if (cmd->scsi_status = SAM_STAT_CHECK_CONDITION)
>  			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  		goto out;

IIUC, this still leaves compare_and_write_post() with a potential NULL
post_ret deref. My preference would be to just go back to having the
unused variable provided by the transport_generic_request_failure()
caller.

Cheers, David

Re: [PATCH] target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()

[David Disseldorp]

On Mon, 5 Nov 2018 18:55:23 +0100, David Disseldorp wrote:

> IIUC, this still leaves compare_and_write_post() with a potential NULL
> post_ret deref.

Actually, I missed that aa73237dcb2d also added the if(success) check
there. Either way, I find your latter fix easier to follow.

Cheers, David

Re: [PATCH] target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()

[Bart Van Assche]

On Mon, 2018-11-05 at 18:55 +-0100, David Disseldorp wrote:
+AD4 Hi Bart,
+AD4 
+AD4 On Mon,  5 Nov 2018 09:23:17 -0800, Bart Van Assche wrote:
+AD4 
+AD4 +AD4 Fixes: aa73237dcb2d (+ACI-scsi: target/core: Always call transport+AF8-complete+AF8-callback() upon failure+ACI)
+AD4 
+AD4 ...
+AD4 +AD4 This patch fixes a bug that was introduced during the merge window. Please consider
+AD4 +AD4 this patch for a v4.20-rc+ADw-n+AD4 kernel.
+AD4 +AD4 
+AD4 +AD4  drivers/target/target+AF8-core+AF8-sbc.c +AHw 3 +-+--
+AD4 +AD4  1 file changed, 2 insertions(+-), 1 deletion(-)
+AD4 +AD4 
+AD4 +AD4 diff --git a/drivers/target/target+AF8-core+AF8-sbc.c b/drivers/target/target+AF8-core+AF8-sbc.c
+AD4 +AD4 index 1ac1f7d2e6c9..7c8433e32906 100644
+AD4 +AD4 --- a/drivers/target/target+AF8-core+AF8-sbc.c
+AD4 +AD4 +-+-+- b/drivers/target/target+AF8-core+AF8-sbc.c
+AD4 +AD4 +AEAAQA -480,7 +-480,8 +AEAAQA static sense+AF8-reason+AF8-t compare+AF8-and+AF8-write+AF8-callback(struct se+AF8-cmd +ACo-cmd, bool succes
+AD4 +AD4  	if (cmd-+AD4-scsi+AF8-status) +AHs
+AD4 +AD4  		pr+AF8-debug(+ACI-compare+AF8-and+AF8-write+AF8-callback: non zero scsi+AF8-status:+ACI
+AD4 +AD4  			+ACI 0x+ACU-02x+AFw-n+ACI, cmd-+AD4-scsi+AF8-status)+ADs
+AD4 +AD4 -		+ACo-post+AF8-ret +AD0 1+ADs
+AD4 +AD4 +-		if (post+AF8-ret)
+AD4 +AD4 +-			+ACo-post+AF8-ret +AD0 1+ADs
+AD4 +AD4  		if (cmd-+AD4-scsi+AF8-status +AD0APQ SAM+AF8-STAT+AF8-CHECK+AF8-CONDITION)
+AD4 +AD4  			ret +AD0 TCM+AF8-LOGICAL+AF8-UNIT+AF8-COMMUNICATION+AF8-FAILURE+ADs
+AD4 +AD4  		goto out+ADs
+AD4 
+AD4 IIUC, this still leaves compare+AF8-and+AF8-write+AF8-post() with a potential NULL
+AD4 post+AF8-ret deref. My preference would be to just go back to having the
+AD4 unused variable provided by the transport+AF8-generic+AF8-request+AF8-failure()
+AD4 caller.

Hi David,

A second version that follows your proposal but that does not reintroduce the
+ACI-unused variable+ACI warning has been posted. Further feedback is welcome.

Bart.