[PATCH] zfcp: fix sense_buffer access bug

[PATCH] zfcp: fix sense_buffer access bug

[FUJITA Tomonori]

The commit de25deb18016f66dcdede165d07654559bb332bc changed
scsi_cmnd.sense_buffer from a static array to a dynamically allocated
buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/s390/scsi/zfcp_fsf.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index fe57941..a9a147d 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -4224,10 +4224,10 @@ zfcp_fsf_send_fcp_command_task_handler(struct zfcp_fsf_req *fsf_req)
 
 		ZFCP_LOG_TRACE("%i bytes sense data provided by FCP\n",
 			       fcp_rsp_iu->fcp_sns_len);
-		memcpy(&scpnt->sense_buffer,
+		memcpy(scpnt->sense_buffer,
 		       zfcp_get_fcp_sns_info_ptr(fcp_rsp_iu), sns_len);
 		ZFCP_HEX_DUMP(ZFCP_LOG_LEVEL_TRACE,
-			      (void *) &scpnt->sense_buffer, sns_len);
+			      (void *)scpnt->sense_buffer, sns_len);
 	}
 
 	/* check for overrun */
-- 
1.5.3.4

Re: [PATCH] zfcp: fix sense_buffer access bug

[Christof Schmitt]

On Sun, Jan 27, 2008 at 12:41:50PM +0900, FUJITA Tomonori wrote:
> The commit de25deb18016f66dcdede165d07654559bb332bc changed
> scsi_cmnd.sense_buffer from a static array to a dynamically allocated
> buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/s390/scsi/zfcp_fsf.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index fe57941..a9a147d 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -4224,10 +4224,10 @@ zfcp_fsf_send_fcp_command_task_handler(struct zfcp_fsf_req *fsf_req)
> 
>  		ZFCP_LOG_TRACE("%i bytes sense data provided by FCP\n",
>  			       fcp_rsp_iu->fcp_sns_len);
> -		memcpy(&scpnt->sense_buffer,
> +		memcpy(scpnt->sense_buffer,
>  		       zfcp_get_fcp_sns_info_ptr(fcp_rsp_iu), sns_len);
>  		ZFCP_HEX_DUMP(ZFCP_LOG_LEVEL_TRACE,
> -			      (void *) &scpnt->sense_buffer, sns_len);
> +			      (void *)scpnt->sense_buffer, sns_len);
>  	}
> 
>  	/* check for overrun */

ACK for fixing the access to the sense buffer.

We are working internally on cleaning up the zfcp messages. With this
change, the 'trace' and 'hex dump' messages will disappear. So, could
you simply remove the ZFCP_HEX_DUMP message above, instead of fixing
it?

Christof

Re: [PATCH] zfcp: fix sense_buffer access bug

[FUJITA Tomonori]

On Mon, 28 Jan 2008 08:46:25 +0100
Christof Schmitt <christof.schmitt@de.ibm.com> wrote:

> On Sun, Jan 27, 2008 at 12:41:50PM +0900, FUJITA Tomonori wrote:
> > The commit de25deb18016f66dcdede165d07654559bb332bc changed
> > scsi_cmnd.sense_buffer from a static array to a dynamically allocated
> > buffer. We can't access to sense_buffer in '&cmd->sense_buffer' way.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  drivers/s390/scsi/zfcp_fsf.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> > index fe57941..a9a147d 100644
> > --- a/drivers/s390/scsi/zfcp_fsf.c
> > +++ b/drivers/s390/scsi/zfcp_fsf.c
> > @@ -4224,10 +4224,10 @@ zfcp_fsf_send_fcp_command_task_handler(struct zfcp_fsf_req *fsf_req)
> > 
> >  		ZFCP_LOG_TRACE("%i bytes sense data provided by FCP\n",
> >  			       fcp_rsp_iu->fcp_sns_len);
> > -		memcpy(&scpnt->sense_buffer,
> > +		memcpy(scpnt->sense_buffer,
> >  		       zfcp_get_fcp_sns_info_ptr(fcp_rsp_iu), sns_len);
> >  		ZFCP_HEX_DUMP(ZFCP_LOG_LEVEL_TRACE,
> > -			      (void *) &scpnt->sense_buffer, sns_len);
> > +			      (void *)scpnt->sense_buffer, sns_len);
> >  	}
> > 
> >  	/* check for overrun */
> 
> ACK for fixing the access to the sense buffer.
> 
> We are working internally on cleaning up the zfcp messages. With this
> change, the 'trace' and 'hex dump' messages will disappear. So, could
> you simply remove the ZFCP_HEX_DUMP message above, instead of fixing
> it?

I can but James has already merged the above patch to scsi-misc. So it
would be more convenient for everyone if you could rebase your
patchset on top of scsi-misc?

Re: [PATCH] zfcp: fix sense_buffer access bug

[Christof Schmitt]

On Mon, Jan 28, 2008 at 06:29:12PM +0900, FUJITA Tomonori wrote:
> > ACK for fixing the access to the sense buffer.
> > 
> > We are working internally on cleaning up the zfcp messages. With this
> > change, the 'trace' and 'hex dump' messages will disappear. So, could
> > you simply remove the ZFCP_HEX_DUMP message above, instead of fixing
> > it?
> 
> I can but James has already merged the above patch to scsi-misc. So it
> would be more convenient for everyone if you could rebase your
> patchset on top of scsi-misc?

Ok, if the pach is already merged, i am fine with that. We will have
to rebase the patches we are working on anyway. Thanks for providing
the fix.

Christof

Re: [PATCH] zfcp: fix sense_buffer access bug

[James Bottomley]

On Mon, 2008-01-28 at 11:04 +0100, Christof Schmitt wrote:
> On Mon, Jan 28, 2008 at 06:29:12PM +0900, FUJITA Tomonori wrote:
> > > ACK for fixing the access to the sense buffer.
> > > 
> > > We are working internally on cleaning up the zfcp messages. With this
> > > change, the 'trace' and 'hex dump' messages will disappear. So, could
> > > you simply remove the ZFCP_HEX_DUMP message above, instead of fixing
> > > it?
> > 
> > I can but James has already merged the above patch to scsi-misc. So it
> > would be more convenient for everyone if you could rebase your
> > patchset on top of scsi-misc?
> 
> Ok, if the pach is already merged, i am fine with that. We will have
> to rebase the patches we are working on anyway. Thanks for providing
> the fix.

It's in my upstream tree, but not merged with Linus and I can easily
rebase.  However, it is a critical bug fix, without it the driver will
likely oops from overwriting command structures, so I'd rather keep it
separate from any cleanup patches.

James