sg: relax 16 byte cdb restriction

PATCH Superseded Douglas Gilbert 2 participants 3 messages Oct 30, 2013 – Dec 1, 2013

Subsystems (2)

SCSI SCSI SG driver

lore

Share & tools

lore

Douglas Gilbert

author maintainer

2013-10-30 22:26 UTC #0 lore raw

PATCH v2 1/1 sg: relax 16 byte cdb restriction

This is essentially the same patch sent 6 weeks ago:
http://marc.info/?l=linux-scsi&m=137943733409512&w=2
re-based on '[PATCH v2] sg: O_EXCL and other lock handling'.

ChangeLog:
- remove the 16 byte CDB (SCSI command) length limit from the sg driver by handling longer CDBs the same way as the bsg driver. Remove comment from sg.h public interface about the cmd_len field being limited to 16 bytes.

Signed-off-by Douglas Gilbert <[email protected]>

drivers/scsi/sg.c +21 -5

index 99c643f..4d434b9 100644
@@ -62,7 +62,7 @@ static int sg_version_num = 30535;	/* 2 digits for each component */
 
 #ifdef CONFIG_SCSI_PROC_FS
 #include <linux/proc_fs.h>
-static char *sg_version_date = "20131029";
+static char *sg_version_date = "20131030";
 
 static int sg_proc_init(void);
 static void sg_proc_cleanup(void);

@@ -72,6 +72,9 @@ static void sg_proc_cleanup(void);
 
 #define SG_MAX_DEVS 32768
 
+#define SG_MAX_CDB_SIZE 255	/* should be 260: spc4r36i 3.1.30 */
+
+
 /*
  * Suppose you want to calculate the formula muldiv(x,m,d)=int(x * m / d)
  * Then when using 32 bit integers x * m may overflow during the calculation.

@@ -574,7 +577,7 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	Sg_request *srp;
 	struct sg_header old_hdr;
 	sg_io_hdr_t *hp;
-	unsigned char cmnd[MAX_COMMAND_SIZE];
+	unsigned char cmnd[SG_MAX_CDB_SIZE];
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;

@@ -606,7 +609,7 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	buf += SZ_SG_HEADER;
 	__get_user(opcode, buf);
 	if (sfp->next_cmd_len > 0) {
-		if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
+		if (sfp->next_cmd_len > SG_MAX_CDB_SIZE) {
 			SCSI_LOG_TIMEOUT(1, printk("sg_write: command length too long\n"));
 			sfp->next_cmd_len = 0;
 			sg_remove_request(sfp, srp);

@@ -683,7 +686,7 @@ sg_new_write(Sg_fd *sfp, struct file *file, const char __user *buf,
 	int k;
 	Sg_request *srp;
 	sg_io_hdr_t *hp;
-	unsigned char cmnd[MAX_COMMAND_SIZE];
+	unsigned char cmnd[SG_MAX_CDB_SIZE];
 	int timeout;
 	unsigned long ul_timeout;

@@ -1659,14 +1662,25 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
 	struct request_queue *q = sfp->parentdp->device->request_queue;
 	struct rq_map_data *md, map_data;
 	int rw = hp->dxfer_direction == SG_DXFER_TO_DEV ? WRITE : READ;
+	unsigned char * long_cmdp = NULL;
 
 	SCSI_LOG_TIMEOUT(4, printk(KERN_INFO "sg_start_req: dxfer_len=%d\n",
 				   dxfer_len));
+	if (hp->cmd_len > BLK_MAX_CDB) {
+		long_cmdp = kzalloc(hp->cmd_len, GFP_KERNEL);
+		if (!long_cmdp)
+			return -ENOMEM;
+	}
 
 	rq = blk_get_request(q, rw, GFP_ATOMIC);
-	if (!rq)
+	if (!rq) {
+		if (long_cmdp)
+			kfree(long_cmdp);
 		return -ENOMEM;
+	}
 
+	if (hp->cmd_len > BLK_MAX_CDB)
+		rq->cmd = long_cmdp;
 	memcpy(rq->cmd, cmd, hp->cmd_len);
 
 	rq->cmd_len = hp->cmd_len;

@@ -1753,6 +1767,8 @@ static int sg_finish_rem_req(Sg_request * srp)
 		if (srp->bio)
 			ret = blk_rq_unmap_user(srp->bio);
 
+		if (srp->rq->cmd != srp->rq->__cmd)
+			kfree(srp->rq->cmd);
 		blk_put_request(srp->rq);
 	}

include/scsi/sg.h +3 -3

index a9f3c6f..18d4aaa 100644
@@ -11,9 +11,9 @@
 Original driver (sg.h):
 *       Copyright (C) 1992 Lawrence Foard
 Version 2 and 3 extensions to driver:
-*       Copyright (C) 1998 - 2006 Douglas Gilbert
+*       Copyright (C) 1998 - 2013 Douglas Gilbert
 
-    Version: 3.5.34 (20060920)
+    Version: 3.5.35 (20130930)
     This version is for 2.6 series kernels.
 
     For a full changelog see http://www.torque.net/sg

@@ -87,7 +87,7 @@ typedef struct sg_io_hdr
 {
     int interface_id;           /* [i] 'S' for SCSI generic (required) */
     int dxfer_direction;        /* [i] data transfer direction  */
-    unsigned char cmd_len;      /* [i] SCSI command length ( <= 16 bytes) */
+    unsigned char cmd_len;      /* [i] SCSI command length */
     unsigned char mx_sb_len;    /* [i] max length to write to sbp */
     unsigned short iovec_count; /* [i] 0 implies no scatter gather */
     unsigned int dxfer_len;     /* [i] byte count of data transfer */

James Bottomley

maintainer

2013-11-30 22:35 UTC #1 lore raw

Quoted 28 lines · Douglas Gilbert

This is essentially the same patch sent 6 weeks ago:
   http://marc.info/?l=linux-scsi&m=137943733409512&w=2
re-based on '[PATCH v2] sg: O_EXCL and other lock handling'.
ChangeLog:
- remove the 16 byte CDB (SCSI command) length limit from the sg driver by handling longer CDBs the same way as the bsg driver. Remove comment from sg.h public interface about the cmd_len field being limited to 16 bytes.
Signed-off-by Douglas Gilbert <[email protected]>
drivers/scsi/sg.c +0 -0
index 99c643f..4d434b9 100644
@@ -62,7 +62,7 @@ static int sg_version_num = 30535;    /* 2 digits
for each component */
#ifdef CONFIG_SCSI_PROC_FS
#include <linux/proc_fs.h>
drivers/scsi/sg.c +1 -1
-static char *sg_version_date = "20131029";
+static char *sg_version_date = "20131030";
 
 static int sg_proc_init(void);
 static void sg_proc_cleanup(void);

drivers/scsi/sg.c static void sg_proc_cleanup(void); · line 75

Quoted 4 lines

drivers/scsi/sg.c +1 -0

@@ -72,6 +72,9 @@ static void sg_proc_cleanup(void);
 
 #define SG_MAX_DEVS 32768
 
+#define SG_MAX_CDB_SIZE 255    /* should be 260: spc4r36i 3.1.30 */

This comment doesn't really make sense to the reader: if you mean the value should be 260 but it can't be set that high because command length is stored in a u8 in the code, say so.

Quoted 17 lines

drivers/scsi/sg.c +0 -0

 /*
  * Suppose you want to calculate the formula muldiv(x,m,d)=int(x *
 m / d)
  * Then when using 32 bit integers x * m may overflow during the

calculation.

drivers/scsi/sg.c +1 -1

@@ -574,7 +577,7 @@ sg_write(struct file *filp, const char __user
 *buf, size_t count, loff_t * ppos)
        Sg_request *srp;
        struct sg_header old_hdr;
        sg_io_hdr_t *hp;
-       unsigned char cmnd[MAX_COMMAND_SIZE];
+       unsigned char cmnd[SG_MAX_CDB_SIZE];
 
        if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp =

sfp->parentdp)))

                return -ENXIO;

drivers/scsi/sg.c sg_write(struct file *filp, const char __user · line 613

Quoted 6 lines

drivers/scsi/sg.c +1 -1

@@ -606,7 +609,7 @@ sg_write(struct file *filp, const char __user
 *buf, size_t count, loff_t * ppos)
        buf += SZ_SG_HEADER;
        __get_user(opcode, buf);
        if (sfp->next_cmd_len > 0) {
-               if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
+               if (sfp->next_cmd_len > SG_MAX_CDB_SIZE) {

This comparison is now impossible (and versions of gcc will warn about it), just remove it.

James

Douglas Gilbert

author maintainer

2013-12-01 16:10 UTC #2 lore raw

Quoted 51 lines · James Bottomley

Quoted 28 lines · Douglas Gilbert
This is essentially the same patch sent 6 weeks ago:
    http://marc.info/?l=linux-scsi&m=137943733409512&w=2
re-based on '[PATCH v2] sg: O_EXCL and other lock handling'.
ChangeLog:
- remove the 16 byte CDB (SCSI command) length limit from the sg driver by handling longer CDBs the same way as the bsg driver. Remove comment from sg.h public interface about the cmd_len field being limited to 16 bytes.
Signed-off-by Douglas Gilbert <[email protected]>
drivers/scsi/sg.c +0 -0
index 99c643f..4d434b9 100644
@@ -62,7 +62,7 @@ static int sg_version_num = 30535;    /* 2 digits
for each component */
  #ifdef CONFIG_SCSI_PROC_FS
  #include <linux/proc_fs.h>
drivers/scsi/sg.c +1 -1
-static char *sg_version_date = "20131029";
+static char *sg_version_date = "20131030";
 
  static int sg_proc_init(void);
  static void sg_proc_cleanup(void);
drivers/scsi/sg.c static void sg_proc_cleanup(void); · line 75
Quoted 4 lines
drivers/scsi/sg.c +1 -0
@@ -72,6 +72,9 @@ static void sg_proc_cleanup(void);
 
  #define SG_MAX_DEVS 32768
 
+#define SG_MAX_CDB_SIZE 255    /* should be 260: spc4r36i 3.1.30 */
This comment doesn't really make sense to the reader: if you mean the value should be 260 but it can't be set that high because command length is stored in a u8 in the code, say so.
Quoted 13 lines
drivers/scsi/sg.c +0 -0
  /*
   * Suppose you want to calculate the formula muldiv(x,m,d)=int(x *
 m / d)
   * Then when using 32 bit integers x * m may overflow during the
calculation.
drivers/scsi/sg.c +1 -1
@@ -574,7 +577,7 @@ sg_write(struct file *filp, const char __user
 *buf, size_t count, loff_t * ppos)
         Sg_request *srp;
         struct sg_header old_hdr;
         sg_io_hdr_t *hp;
-       unsigned char cmnd[MAX_COMMAND_SIZE];
+       unsigned char cmnd[SG_MAX_CDB_SIZE];
 

Quoted 13 lines

drivers/scsi/sg.c sg_write(struct file *filp, const char __user · line 613

Quoted 10 lines

drivers/scsi/sg.c +0 -0

         if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp =

sfp->parentdp)))

                 return -ENXIO;

drivers/scsi/sg.c +1 -1

@@ -606,7 +609,7 @@ sg_write(struct file *filp, const char __user
 *buf, size_t count, loff_t * ppos)
         buf += SZ_SG_HEADER;
         __get_user(opcode, buf);
         if (sfp->next_cmd_len > 0) {
-               if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
+               if (sfp->next_cmd_len > SG_MAX_CDB_SIZE) {

This comparison is now impossible (and versions of gcc will warn about it), just remove it.

Instead of v2 perhaps you might have reviewed v3 of this patch dated 20131113 and the associated v3 O_EXCL and other lock handling patch. Anyway the same comments apply so v4 of this patch is coming.

Doug Gilbert