diff mbox

multipath-tools: mismatch between CDB allocation length and SG_IO dxfer_len

Message ID 20140405131323.00000d4b@localhost (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show

Commit Message

Sebastian Herbszt April 5, 2014, 11:13 a.m. UTC 
Hello,

while playing with multipath-tools and scst I got the following warning:

[ 6382.467569] [3806]: scst: scst_parse_cmd:826:Warning: expected
transfer length 256 for opcode 0x12 (handler vdisk_blockio, target
scst_local) doesn't match decoded value 128
[ 6382.467574] [3806]: scst_parse_cmd:828:Suspicious CDB:
[ 6382.467579]  (h)___0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
[ 6382.467581]    0: 12 01 c0 00 80 00   ......

This happened when my scst target pretended to be a CLARiiON.
I noticed a mismatch in emc_clariion_prio (libmultipath/prioritizers/emc.c)
between the allocation length and dxfer_len:

int emc_clariion_prio(const char *dev, int fd)
{
        unsigned char sense_buffer[256];
        unsigned char sb[128];
        unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
                                                sizeof(sb), 0};

...

        memset(&sense_buffer, 0, 256);
        io_hdr.interface_id = 'S';
        io_hdr.cmd_len = sizeof (inqCmdBlk);
        io_hdr.mx_sb_len = sizeof (sb);
        io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
        io_hdr.dxfer_len = sizeof (sense_buffer);
        io_hdr.dxferp = sense_buffer;
        io_hdr.cmdp = inqCmdBlk;
        io_hdr.sbp = sb;
        io_hdr.timeout = 60000;
        io_hdr.pack_id = 0;

The following change shrinks the size of sense_buffer to 128.

Sebastian


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Sebastian Herbszt May 21, 2014, 11:14 a.m. UTC | #1 
> Hello,
> 
> while playing with multipath-tools and scst I got the following warning:
> 
> [ 6382.467569] [3806]: scst: scst_parse_cmd:826:Warning: expected
> transfer length 256 for opcode 0x12 (handler vdisk_blockio, target
> scst_local) doesn't match decoded value 128
> [ 6382.467574] [3806]: scst_parse_cmd:828:Suspicious CDB:
> [ 6382.467579]  (h)___0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
> [ 6382.467581]    0: 12 01 c0 00 80 00   ......
> 
> This happened when my scst target pretended to be a CLARiiON.
> I noticed a mismatch in emc_clariion_prio (libmultipath/prioritizers/emc.c)
> between the allocation length and dxfer_len:
> 
> int emc_clariion_prio(const char *dev, int fd)
> {
>         unsigned char sense_buffer[256];
>         unsigned char sb[128];
>         unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
>                                                 sizeof(sb), 0};
> 
> ...
> 
>         memset(&sense_buffer, 0, 256);
>         io_hdr.interface_id = 'S';
>         io_hdr.cmd_len = sizeof (inqCmdBlk);
>         io_hdr.mx_sb_len = sizeof (sb);
>         io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
>         io_hdr.dxfer_len = sizeof (sense_buffer);
>         io_hdr.dxferp = sense_buffer;
>         io_hdr.cmdp = inqCmdBlk;
>         io_hdr.sbp = sb;
>         io_hdr.timeout = 60000;
>         io_hdr.pack_id = 0;
> 
> The following change shrinks the size of sense_buffer to 128.
> 
> Sebastian
> 
> diff --git a/libmultipath/prioritizers/emc.c b/libmultipath/prioritizers/emc.c
> index 87e9a8d..91b3d90 100644
> --- a/libmultipath/prioritizers/emc.c
> +++ b/libmultipath/prioritizers/emc.c
> @@ -14,15 +14,15 @@
>  
>  int emc_clariion_prio(const char *dev, int fd)
>  {
> -	unsigned char sense_buffer[256];
> +	unsigned char sense_buffer[128];
>  	unsigned char sb[128];
>  	unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
> -						sizeof(sb), 0};
> +						sizeof(sense_buffer), 0};
>  	struct sg_io_hdr io_hdr;
>  	int ret = PRIO_UNDEF;
>  
>  	memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
> -	memset(&sense_buffer, 0, 256);
> +	memset(&sense_buffer, 0, 128);
>  	io_hdr.interface_id = 'S';
>  	io_hdr.cmd_len = sizeof (inqCmdBlk);
>  	io_hdr.mx_sb_len = sizeof (sb);

Ping! Any comments?

Sebastian

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Christophe Varoqui May 21, 2014, 8:31 p.m. UTC | #2 
Hi,

have you verified this part is not enough to fix the problem, without
shrinking the response size to 128 ?

        unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
-                                               sizeof(sb), 0};
+                                               sizeof(sense_buffer), 0};

Is it me or the variable name is just plain misleading ?

Best regards,
Christophe Varoqui
www.opensvc.com



On Sat, Apr 5, 2014 at 1:13 PM, Sebastian Herbszt <herbszt@gmx.de> wrote:

> Hello,
>
> while playing with multipath-tools and scst I got the following warning:
>
> [ 6382.467569] [3806]: scst: scst_parse_cmd:826:Warning: expected
> transfer length 256 for opcode 0x12 (handler vdisk_blockio, target
> scst_local) doesn't match decoded value 128
> [ 6382.467574] [3806]: scst_parse_cmd:828:Suspicious CDB:
> [ 6382.467579]  (h)___0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
> [ 6382.467581]    0: 12 01 c0 00 80 00   ......
>
> This happened when my scst target pretended to be a CLARiiON.
> I noticed a mismatch in emc_clariion_prio (libmultipath/prioritizers/emc.c)
> between the allocation length and dxfer_len:
>
> int emc_clariion_prio(const char *dev, int fd)
> {
>         unsigned char sense_buffer[256];
>         unsigned char sb[128];
>         unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
>                                                 sizeof(sb), 0};
>
> ...
>
>         memset(&sense_buffer, 0, 256);
>         io_hdr.interface_id = 'S';
>         io_hdr.cmd_len = sizeof (inqCmdBlk);
>         io_hdr.mx_sb_len = sizeof (sb);
>         io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
>         io_hdr.dxfer_len = sizeof (sense_buffer);
>         io_hdr.dxferp = sense_buffer;
>         io_hdr.cmdp = inqCmdBlk;
>         io_hdr.sbp = sb;
>         io_hdr.timeout = 60000;
>         io_hdr.pack_id = 0;
>
> The following change shrinks the size of sense_buffer to 128.
>
> Sebastian
>
> diff --git a/libmultipath/prioritizers/emc.c
> b/libmultipath/prioritizers/emc.c
> index 87e9a8d..91b3d90 100644
> --- a/libmultipath/prioritizers/emc.c
> +++ b/libmultipath/prioritizers/emc.c
> @@ -14,15 +14,15 @@
>
>  int emc_clariion_prio(const char *dev, int fd)
>  {
> -       unsigned char sense_buffer[256];
> +       unsigned char sense_buffer[128];
>         unsigned char sb[128];
>         unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
> -                                               sizeof(sb), 0};
> +                                               sizeof(sense_buffer), 0};
>         struct sg_io_hdr io_hdr;
>         int ret = PRIO_UNDEF;
>
>         memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
> -       memset(&sense_buffer, 0, 256);
> +       memset(&sense_buffer, 0, 128);
>         io_hdr.interface_id = 'S';
>         io_hdr.cmd_len = sizeof (inqCmdBlk);
>         io_hdr.mx_sb_len = sizeof (sb);
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Sebastian Herbszt May 21, 2014, 10:46 p.m. UTC | #3 
> Hi,
> 
> have you verified this part is not enough to fix the problem, without
> shrinking the response size to 128 ?
> 
>         unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
> -                                               sizeof(sb), 0};
> +                                               sizeof(sense_buffer), 0};

sizeof(sense_buffer) is 256 and it gets truncated to 0:

ioctl(4, SG_IO, {'S', SG_DXFER_FROM_DEV, cmd[6]=[12, 01, c0, 00, 00, 00], ...

[ 3730.100199] [5674]: scst: scst_parse_cmd:828:Warning: expected transfer length 256 for opcode INQUIRY (handler vdisk_blockio, target scst_local) doesn't match decoded value 0
[ 3730.100203] [5674]: scst_parse_cmd:830:Suspicious CDB:
[ 3730.100204]  (h)___0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
[ 3730.100206]    0: 12 01 c0 00 00 00   ......

This works

@@ -16,8 +16,9 @@ int emc_clariion_prio(const char *dev, int fd)
 {
        unsigned char sense_buffer[256];
        unsigned char sb[128];
-       unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
-                                               sizeof(sb), 0};
+       unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0,
+                                       (sizeof(sense_buffer) >> 8) & 0xff,
+                                       sizeof(sense_buffer) & 0xff,  0};
        struct sg_io_hdr io_hdr;
        int ret = PRIO_UNDEF;

but I am not sure a buffer of 256 is required, because the checker just uses
128 bytes for the same page.

> Is it me or the variable name is just plain misleading ?

Should be "resp" or something like that.

> Best regards,
> Christophe Varoqui
> www.opensvc.com

Please keep me CC'ed in replies.

Sebastian

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Christophe Varoqui June 10, 2014, 5:58 a.m. UTC | #4 
Applied.

I received the same patch for the rdac prioritizer
 from netapp. Democratically, you're right.

Regards,
Christophe Varoqui
www.opensvc.com


On Sat, Apr 5, 2014 at 1:13 PM, Sebastian Herbszt <herbszt@gmx.de> wrote:

> Hello,
>
> while playing with multipath-tools and scst I got the following warning:
>
> [ 6382.467569] [3806]: scst: scst_parse_cmd:826:Warning: expected
> transfer length 256 for opcode 0x12 (handler vdisk_blockio, target
> scst_local) doesn't match decoded value 128
> [ 6382.467574] [3806]: scst_parse_cmd:828:Suspicious CDB:
> [ 6382.467579]  (h)___0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
> [ 6382.467581]    0: 12 01 c0 00 80 00   ......
>
> This happened when my scst target pretended to be a CLARiiON.
> I noticed a mismatch in emc_clariion_prio (libmultipath/prioritizers/emc.c)
> between the allocation length and dxfer_len:
>
> int emc_clariion_prio(const char *dev, int fd)
> {
>         unsigned char sense_buffer[256];
>         unsigned char sb[128];
>         unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
>                                                 sizeof(sb), 0};
>
> ...
>
>         memset(&sense_buffer, 0, 256);
>         io_hdr.interface_id = 'S';
>         io_hdr.cmd_len = sizeof (inqCmdBlk);
>         io_hdr.mx_sb_len = sizeof (sb);
>         io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
>         io_hdr.dxfer_len = sizeof (sense_buffer);
>         io_hdr.dxferp = sense_buffer;
>         io_hdr.cmdp = inqCmdBlk;
>         io_hdr.sbp = sb;
>         io_hdr.timeout = 60000;
>         io_hdr.pack_id = 0;
>
> The following change shrinks the size of sense_buffer to 128.
>
> Sebastian
>
> diff --git a/libmultipath/prioritizers/emc.c
> b/libmultipath/prioritizers/emc.c
> index 87e9a8d..91b3d90 100644
> --- a/libmultipath/prioritizers/emc.c
> +++ b/libmultipath/prioritizers/emc.c
> @@ -14,15 +14,15 @@
>
>  int emc_clariion_prio(const char *dev, int fd)
>  {
> -       unsigned char sense_buffer[256];
> +       unsigned char sense_buffer[128];
>         unsigned char sb[128];
>         unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
> -                                               sizeof(sb), 0};
> +                                               sizeof(sense_buffer), 0};
>         struct sg_io_hdr io_hdr;
>         int ret = PRIO_UNDEF;
>
>         memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
> -       memset(&sense_buffer, 0, 256);
> +       memset(&sense_buffer, 0, 128);
>         io_hdr.interface_id = 'S';
>         io_hdr.cmd_len = sizeof (inqCmdBlk);
>         io_hdr.mx_sb_len = sizeof (sb);
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

diff --git a/libmultipath/prioritizers/emc.c b/libmultipath/prioritizers/emc.c
index 87e9a8d..91b3d90 100644
--- a/libmultipath/prioritizers/emc.c
+++ b/libmultipath/prioritizers/emc.c
@@ -14,15 +14,15 @@ 
 
 int emc_clariion_prio(const char *dev, int fd)
 {
-	unsigned char sense_buffer[256];
+	unsigned char sense_buffer[128];
 	unsigned char sb[128];
 	unsigned char inqCmdBlk[INQUIRY_CMDLEN] = {INQUIRY_CMD, 1, 0xC0, 0,
-						sizeof(sb), 0};
+						sizeof(sense_buffer), 0};
 	struct sg_io_hdr io_hdr;
 	int ret = PRIO_UNDEF;
 
 	memset(&io_hdr, 0, sizeof (struct sg_io_hdr));
-	memset(&sense_buffer, 0, 256);
+	memset(&sense_buffer, 0, 128);
 	io_hdr.interface_id = 'S';
 	io_hdr.cmd_len = sizeof (inqCmdBlk);
 	io_hdr.mx_sb_len = sizeof (sb);