target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()
diff mbox series

Message ID 20181105172317.248955-1-bvanassche@acm.org
State New, archived
Headers show
Series
  • target/core: Make it safe to pass NULL as third argument to compare_and_write_callback()
Related show

Commit Message

Bart Van Assche Nov. 5, 2018, 5:23 p.m. UTC
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(-)

Comments

David Disseldorp Nov. 5, 2018, 5:55 p.m. UTC | #1
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
David Disseldorp Nov. 5, 2018, 7:09 p.m. UTC | #2
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
Bart Van Assche Nov. 5, 2018, 7:15 p.m. UTC | #3
On Mon, 2018-11-05 at 18:55 +0100, David Disseldorp wrote:
> 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.

Hi David,

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

Bart.

Patch
diff mbox series

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;