Xen devel - Jul 2008 - [PATCH 1/4] pvSCSI : Add white list to SCSI command emulation

Add "white list" control to SCSI command emulation. Current setting
allows following mandatory and safe commands.

TEST UNIT READY
REZERO UNIT
REQUEST SENSE
FORMAT UNIT
READ BLOCK LIMITS
READ(06)
WRITE(06)
WRITE FILEMARKS
SPACE
INQUIRY
ERASE
MODE SENSE(06)
SEND DIAGNOSTIC
READ CAPACITY
READ(10)
WRITE(10)
REPORT LUN


Signed-off-by: Tomonari Horikoshi <t.horikoshi@jp.fujitsu.com>
Signed-off-by: Jun Kamada <kama@jp.fujitsu.com>

-----
Jun Kamada


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> +	bitmap[ERASE] = VSCSIIF_NEED_CMD_EXEC;
> +	pre_function[ERASE] = NULL;
> +	post_function[ERASE] = NULL;
> +
> +	bitmap[MODE_SENSE] = VSCSIIF_NEED_CMD_EXEC;
> +	pre_function[MODE_SENSE] = NULL;
> +	post_function[MODE_SENSE] = NULL;
> +
> +	bitmap[SEND_DIAGNOSTIC] = VSCSIIF_NEED_CMD_EXEC;
> +	pre_function[SEND_DIAGNOSTIC] = NULL;
> +	post_function[SEND_DIAGNOSTIC] = NULL;
Hmm, careful here.  If the DevOffL bit is set on a SEND DIAGNOSTIC
command then it can cause interference in other LUNs on the target,
which would be unfortunate if they''re exposed to different VMs.
> +
> +	bitmap[READ_CAPACITY] = VSCSIIF_NEED_CMD_EXEC;
> +	pre_function[READ_CAPACITY] = NULL;
> +	post_function[READ_CAPACITY] = NULL;
> +
> +	bitmap[READ_10] = VSCSIIF_NEED_CMD_EXEC;
> +	pre_function[READ_10] = NULL;
> +	post_function[READ_10] = NULL;
> +
> +	bitmap[WRITE_10] = VSCSIIF_NEED_CMD_EXEC;
> +	pre_function[WRITE_10] = NULL;
> +	post_function[WRITE_10] = NULL;
Do you know what happens if the SCSI CDB is inconsistent with the
scatter list?  For instance, if the CDB says to read 16 sectors but
the frontend-supplied buffer is only big enough for 8, are we going to
end up DMAing over random memory?

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Steven-san,

On Fri, 4 Jul 2008 17:21:54 +0100
Steven Smith <steven.smith@citrix.com> wrote:
> > +	bitmap[ERASE] = VSCSIIF_NEED_CMD_EXEC;
> > +	pre_function[ERASE] = NULL;
> > +	post_function[ERASE] = NULL;
> > +
> > +	bitmap[MODE_SENSE] = VSCSIIF_NEED_CMD_EXEC;
> > +	pre_function[MODE_SENSE] = NULL;
> > +	post_function[MODE_SENSE] = NULL;
> > +
> > +	bitmap[SEND_DIAGNOSTIC] = VSCSIIF_NEED_CMD_EXEC;
> > +	pre_function[SEND_DIAGNOSTIC] = NULL;
> > +	post_function[SEND_DIAGNOSTIC] = NULL;
> Hmm, careful here.  If the DevOffL bit is set on a SEND DIAGNOSTIC
> command then it can cause interference in other LUNs on the target,
> which would be unfortunate if they''re exposed to different VMs.
> 
> > +
> > +	bitmap[READ_CAPACITY] = VSCSIIF_NEED_CMD_EXEC;
> > +	pre_function[READ_CAPACITY] = NULL;
> > +	post_function[READ_CAPACITY] = NULL;
> > +
> > +	bitmap[READ_10] = VSCSIIF_NEED_CMD_EXEC;
> > +	pre_function[READ_10] = NULL;
> > +	post_function[READ_10] = NULL;
> > +
> > +	bitmap[WRITE_10] = VSCSIIF_NEED_CMD_EXEC;
> > +	pre_function[WRITE_10] = NULL;
> > +	post_function[WRITE_10] = NULL;
> Do you know what happens if the SCSI CDB is inconsistent with the
> scatter list?  For instance, if the CDB says to read 16 sectors but
> the frontend-supplied buffer is only big enough for 8, are we going to
> end up DMAing over random memory?
I consider that native SCSI driver, which generates DMA request, will 
reject such the inconsistent request. The native driver generates 
multiple CDB requests according to the number of scatter/gather 
segments. At the same time, doesn''t the native driver check such the 
inconsistency?
So, if there are any inconsistency, pvSCSI backend driver will just 
get an error from native driver.

Perhaps, do you warry about inconsitency between size of some segment
frontend driver says, and size of the segment the frontend driver
actually allocated?

# Sorry, if I misundastood your question.


Best regards,


-----
Jun Kamada



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > > +	bitmap[READ_CAPACITY] = VSCSIIF_NEED_CMD_EXEC;
> > > +	pre_function[READ_CAPACITY] = NULL;
> > > +	post_function[READ_CAPACITY] = NULL;
> > > +
> > > +	bitmap[READ_10] = VSCSIIF_NEED_CMD_EXEC;
> > > +	pre_function[READ_10] = NULL;
> > > +	post_function[READ_10] = NULL;
> > > +
> > > +	bitmap[WRITE_10] = VSCSIIF_NEED_CMD_EXEC;
> > > +	pre_function[WRITE_10] = NULL;
> > > +	post_function[WRITE_10] = NULL;
> > Do you know what happens if the SCSI CDB is inconsistent with the
> > scatter list?  For instance, if the CDB says to read 16 sectors but
> > the frontend-supplied buffer is only big enough for 8, are we going to
> > end up DMAing over random memory?
> I consider that native SCSI driver, which generates DMA request, will 
> reject such the inconsistent request. The native driver generates 
> multiple CDB requests according to the number of scatter/gather 
> segments. At the same time, doesn''t the native driver check such
the
> inconsistency?
> So, if there are any inconsistency, pvSCSI backend driver will just 
> get an error from native driver.
> 
> Perhaps, do you warry about inconsitency between size of some segment
> frontend driver says, and size of the segment the frontend driver
> actually allocated?
Suppose the frontend generates a READ command with the CDB
TransferLength field set to 128 sectors, but the nr_segments field of
the vscsiif_request_t set to 1.  scsiback will then a scsi_cmnd whose
CDB says to read 128 sectors, but whose associated scatter-gather list
only has room for 8, and pass that off to blk_execute_rq_nowait().
What happens to the other 120 sectors?  Hopefully, something will
detect the inconsistency and make the request fail, but I can''t really
see what.  It would be very bad if the request made it down to the
physical card, which tried to satisfy it by DMAing the 120 sectors
into some random bit of memory.  I''ve had a quick look around
drivers/scsi and I can''t immediately see anything which tries to do
the required kind of consistency checking.  On the other hand, I''d
have expected sg.c to need something, at least, and it doesn''t, so
perhaps it''s safe to rely on the hardware to do the checks.

Actually doing the checking could prove tricky, as well, because of
commands like READ6 where you need to know the type of device which is
being addressed.  For block devices, the command has the logical block
address in bytes 2 and 3, with the transfer length in byte 4, but
streaming devices use all three bytes to encode the length (because
block address is meaningless on stream devices).  That''s going to make
it tricky to design a generic validation routine.  Hmm.

Steven.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel