[PATCH 3/3] tgt: fix scsi command leak

[PATCH 3/3] tgt: fix scsi command leak

[FUJITA Tomonori]

The failure to map user-space pages leads to scsi command leak. It can
happens mostly because of user-space daemon bugs (or OOM). This patch
makes tgt just notify a LLD of the failure with sense when
blk_rq_map_user() fails.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/scsi_tgt_lib.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index dc8781a..c05dff9 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -459,6 +459,16 @@ static struct request *tgt_cmd_hash_look
 	return rq;
 }
 
+static void scsi_tgt_build_sense(unsigned char *sense_buffer, unsigned char key,
+				 unsigned char asc, unsigned char asq)
+{
+	sense_buffer[0] = 0x70;
+	sense_buffer[2] = key;
+	sense_buffer[7] = 0xa;
+	sense_buffer[12] = asc;
+	sense_buffer[13] = asq;
+}
+
 int scsi_tgt_kspace_exec(int host_no, int result, u64 tag,
 			 unsigned long uaddr, u32 len, unsigned long sense_uaddr,
 			 u32 sense_len, u8 rw)
@@ -514,9 +524,16 @@ int scsi_tgt_kspace_exec(int host_no, in
 	if (len) {
 		err = scsi_map_user_pages(rq->end_io_data, cmd, uaddr, len, rw);
 		if (err) {
-			eprintk("%p %d\n", cmd, err);
-			err = -EAGAIN;
-			goto done;
+			/*
+			 * user-space daemon bugs or OOM
+			 * TODO: we can do better for OOM.
+			 */
+			eprintk("cmd %p ret %d uaddr %lx len %d rw %d\n",
+				cmd, err, uaddr, len, rw);
+			cmd->result = SAM_STAT_CHECK_CONDITION;
+			memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+			scsi_tgt_build_sense(cmd->sense_buffer,
+					     HARDWARE_ERROR, 0, 0);
 		}
 	}
 	err = scsi_tgt_transfer_response(cmd);
-- 
1.4.3.2

Re: [PATCH 3/3] tgt: fix scsi command leak

[Douglas Gilbert]

FUJITA Tomonori wrote:
> The failure to map user-space pages leads to scsi command leak. It can
> happens mostly because of user-space daemon bugs (or OOM). This patch
> makes tgt just notify a LLD of the failure with sense when
> blk_rq_map_user() fails.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> ---
>  drivers/scsi/scsi_tgt_lib.c |   23 ++++++++++++++++++++---
>  1 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
> index dc8781a..c05dff9 100644
> --- a/drivers/scsi/scsi_tgt_lib.c
> +++ b/drivers/scsi/scsi_tgt_lib.c
> @@ -459,6 +459,16 @@ static struct request *tgt_cmd_hash_look
>  	return rq;
>  }
>  
> +static void scsi_tgt_build_sense(unsigned char *sense_buffer, unsigned char key,
> +				 unsigned char asc, unsigned char asq)
> +{
> +	sense_buffer[0] = 0x70;
> +	sense_buffer[2] = key;
> +	sense_buffer[7] = 0xa;
> +	sense_buffer[12] = asc;
> +	sense_buffer[13] = asq;
> +}
> +

Tomo,
Perhaps you could add a memset(sense_buffer, 0, 18) before
those assignments and state that this is "fixed" sense
buffer format.

What about an option for descriptor sense format? With SAT now
a standard, we now have one more reason to support
descriptor format when required. The ATA PASS-THROUGH SCSI
commands in SAT use descriptor sense format to return
ATA registers.


<aside>
While on the subject of sense data, I note that the
ATA folks (t13.org) are proposing an "ATA REQUEST
SENSE" command to leverage of existing SCSI
sense_key, asc, ascq tuples.

Doug Gilbert


>  int scsi_tgt_kspace_exec(int host_no, int result, u64 tag,
>  			 unsigned long uaddr, u32 len, unsigned long sense_uaddr,
>  			 u32 sense_len, u8 rw)
> @@ -514,9 +524,16 @@ int scsi_tgt_kspace_exec(int host_no, in
>  	if (len) {
>  		err = scsi_map_user_pages(rq->end_io_data, cmd, uaddr, len, rw);
>  		if (err) {
> -			eprintk("%p %d\n", cmd, err);
> -			err = -EAGAIN;
> -			goto done;
> +			/*
> +			 * user-space daemon bugs or OOM
> +			 * TODO: we can do better for OOM.
> +			 */
> +			eprintk("cmd %p ret %d uaddr %lx len %d rw %d\n",
> +				cmd, err, uaddr, len, rw);
> +			cmd->result = SAM_STAT_CHECK_CONDITION;
> +			memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
> +			scsi_tgt_build_sense(cmd->sense_buffer,
> +					     HARDWARE_ERROR, 0, 0);
>  		}
>  	}
>  	err = scsi_tgt_transfer_response(cmd);

Re: [PATCH 3/3] tgt: fix scsi command leak

[FUJITA Tomonori]

From: Douglas Gilbert <dougg@torque.net>
Subject: Re: [PATCH 3/3] tgt: fix scsi command leak
Date: Sat, 03 Mar 2007 11:58:19 -0500

> FUJITA Tomonori wrote:
> > The failure to map user-space pages leads to scsi command leak. It can
> > happens mostly because of user-space daemon bugs (or OOM). This patch
> > makes tgt just notify a LLD of the failure with sense when
> > blk_rq_map_user() fails.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> > ---
> >  drivers/scsi/scsi_tgt_lib.c |   23 ++++++++++++++++++++---
> >  1 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
> > index dc8781a..c05dff9 100644
> > --- a/drivers/scsi/scsi_tgt_lib.c
> > +++ b/drivers/scsi/scsi_tgt_lib.c
> > @@ -459,6 +459,16 @@ static struct request *tgt_cmd_hash_look
> >  	return rq;
> >  }
> >  
> > +static void scsi_tgt_build_sense(unsigned char *sense_buffer, unsigned char key,
> > +				 unsigned char asc, unsigned char asq)
> > +{
> > +	sense_buffer[0] = 0x70;
> > +	sense_buffer[2] = key;
> > +	sense_buffer[7] = 0xa;
> > +	sense_buffer[12] = asc;
> > +	sense_buffer[13] = asq;
> > +}
> > +
> 
> Tomo,
> Perhaps you could add a memset(sense_buffer, 0, 18) before
> those assignments and state that this is "fixed" sense
> buffer format.

I think that it isn't necessary because when a target mode driver
allocates scsi_cmnd, scsi_host_get_command() does that.


> What about an option for descriptor sense format? With SAT now
> a standard, we now have one more reason to support
> descriptor format when required. The ATA PASS-THROUGH SCSI
> commands in SAT use descriptor sense format to return
> ATA registers.

tgt's kernel-space code doesn't know anything about SCSI devices that
initiators talks to. So it's difficult to send proper sense buffer.
Nomally, we don't have this problem because tgt user-space code builds
sense buffer.

The bug that we are trying to fix is that the scsi command leak due to
the user-space's bugs. So we can't rely on the user-space for this.

Not that, like open-iscsi, the user-space bugs are pretty critical for
tgt as the kernel-space bugs. We don't think target mode drivers can
continue to work. However, tgt should tell target mode drivers that
unrecoverable problems happen and we should cleanly unload the kernel
modules.


> <aside>
> While on the subject of sense data, I note that the
> ATA folks (t13.org) are proposing an "ATA REQUEST
> SENSE" command to leverage of existing SCSI
> sense_key, asc, ascq tuples.

Re: [PATCH 3/3] tgt: fix scsi command leak

[Douglas Gilbert]

FUJITA Tomonori wrote:
> From: Douglas Gilbert <dougg@torque.net>
> Subject: Re: [PATCH 3/3] tgt: fix scsi command leak
> Date: Sat, 03 Mar 2007 11:58:19 -0500
> 
>> FUJITA Tomonori wrote:
>>> The failure to map user-space pages leads to scsi command leak. It can
>>> happens mostly because of user-space daemon bugs (or OOM). This patch
>>> makes tgt just notify a LLD of the failure with sense when
>>> blk_rq_map_user() fails.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
>>> ---
>>>  drivers/scsi/scsi_tgt_lib.c |   23 ++++++++++++++++++++---
>>>  1 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
>>> index dc8781a..c05dff9 100644
>>> --- a/drivers/scsi/scsi_tgt_lib.c
>>> +++ b/drivers/scsi/scsi_tgt_lib.c
>>> @@ -459,6 +459,16 @@ static struct request *tgt_cmd_hash_look
>>>  	return rq;
>>>  }
>>>  
>>> +static void scsi_tgt_build_sense(unsigned char *sense_buffer, unsigned char key,
>>> +				 unsigned char asc, unsigned char asq)
>>> +{
>>> +	sense_buffer[0] = 0x70;
>>> +	sense_buffer[2] = key;
>>> +	sense_buffer[7] = 0xa;
>>> +	sense_buffer[12] = asc;
>>> +	sense_buffer[13] = asq;
>>> +}
>>> +
>> Tomo,
>> Perhaps you could add a memset(sense_buffer, 0, 18) before
>> those assignments and state that this is "fixed" sense
>> buffer format.
> 
> I think that it isn't necessary because when a target mode driver
> allocates scsi_cmnd, scsi_host_get_command() does that.
> 
> 
>> What about an option for descriptor sense format? With SAT now
>> a standard, we now have one more reason to support
>> descriptor format when required. The ATA PASS-THROUGH SCSI
>> commands in SAT use descriptor sense format to return
>> ATA registers.
> 
> tgt's kernel-space code doesn't know anything about SCSI devices that
> initiators talks to. So it's difficult to send proper sense buffer.
> Nomally, we don't have this problem because tgt user-space code builds
> sense buffer.
> 
> The bug that we are trying to fix is that the scsi command leak due to
> the user-space's bugs. So we can't rely on the user-space for this.
> 
> Not that, like open-iscsi, the user-space bugs are pretty critical for
> tgt as the kernel-space bugs. We don't think target mode drivers can
> continue to work. However, tgt should tell target mode drivers that
> unrecoverable problems happen and we should cleanly unload the kernel
> modules.

Tomo,
If I understand correctly, there is a target SCSI command
interpreter in a user space daemon (plus lu support) and the
target transport end point in kernel space (roughly speaking).
So if there is some problem in the kernel module, or
the user space daemon goes away (or won't respond) then what
you have is a transport error at the target end.

The error should be lower level than SCSI commands (i.e.
transport level). The kernel module doesn't know the state
of target SCSI command interpreter (by design). For example
the application client may have set the D_SENSE bit in the
control mode page prior to the failure that your code is
addressing. So the application client won't be expecting
fixed sense data format thereafter.

So what the code is doing is definitely better than nothing,
but IMO it isn't quite right either.

Doug Gilbert

Re: [PATCH 3/3] tgt: fix scsi command leak

[FUJITA Tomonori]

From: Douglas Gilbert <dougg@torque.net>
Subject: Re: [PATCH 3/3] tgt: fix scsi command leak
Date: Mon, 05 Mar 2007 10:36:15 -0500

> >> What about an option for descriptor sense format? With SAT now
> >> a standard, we now have one more reason to support
> >> descriptor format when required. The ATA PASS-THROUGH SCSI
> >> commands in SAT use descriptor sense format to return
> >> ATA registers.
> > 
> > tgt's kernel-space code doesn't know anything about SCSI devices that
> > initiators talks to. So it's difficult to send proper sense buffer.
> > Nomally, we don't have this problem because tgt user-space code builds
> > sense buffer.
> > 
> > The bug that we are trying to fix is that the scsi command leak due to
> > the user-space's bugs. So we can't rely on the user-space for this.
> > 
> > Not that, like open-iscsi, the user-space bugs are pretty critical for
> > tgt as the kernel-space bugs. We don't think target mode drivers can
> > continue to work. However, tgt should tell target mode drivers that
> > unrecoverable problems happen and we should cleanly unload the kernel
> > modules.
> 
> Tomo,
> If I understand correctly, there is a target SCSI command
> interpreter in a user space daemon (plus lu support) and the
> target transport end point in kernel space (roughly speaking).
> So if there is some problem in the kernel module, or
> the user space daemon goes away (or won't respond) then what
> you have is a transport error at the target end.

Yeah.


> The error should be lower level than SCSI commands (i.e.
> transport level). The kernel module doesn't know the state
> of target SCSI command interpreter (by design). For example
> the application client may have set the D_SENSE bit in the
> control mode page prior to the failure that your code is
> addressing. So the application client won't be expecting
> fixed sense data format thereafter.
> 
> So what the code is doing is definitely better than nothing,
> but IMO it isn't quite right either.

It should work. The initiators probably sends tmf requests and finally
gives up the device.

Could a lld do something better if tgt notifies the lld that tgt can't
handle the command via host->transfer_response? Even though the
initiator must give up the device in the end because the user-space
deamon can't continue.

If so, one possible option is that tgt notifies a lld of tgt's
original status via host->transfer_response though currently
host->transfer_response uses only SAM_STAT_*.