Skip to contentSkip to conversation

sg: relax 16 byte cdb restriction

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

Subsystems (2)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Douglas Gilbert

author maintainer
#0 lore raw

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

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.
- remove some dead code caused by this change
- cleanup comment block at the top of sg.h, fix urls

drivers/scsi/sg.c +19 -11
index 843c66e..2e99a4e 100644
@@ -73,6 +73,9 @@ static void sg_proc_cleanup(void);
 
 #define SG_MAX_DEVS 32768
 
+#define SG_MAX_CDB_SIZE 255	/* ideally should be 260 (spc4r36i 3.1.30)
+				 * but limited to unsigned char */
+
 /*
  * 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.
@@ -159,7 +162,7 @@ typedef struct sg_fd {		/* holds the state of a file descriptor */
 	char low_dma;		/* as in parent but possibly overridden to 1 */
 	char force_packid;	/* 1 -> pack_id input to read(), 0 -> ignored */
 	char cmd_q;		/* 1 -> allow command queuing, 0 -> don't */
-	char next_cmd_len;	/* 0 -> automatic (def), >0 -> use on next write() */
+	unsigned char next_cmd_len; /* 0: automatic, >0: use on next write() */
 	char keep_orphan;	/* 0 -> drop orphan (def), 1 -> keep for read() */
 	char mmap_called;	/* 0 -> mmap() never called on this fd */
 	struct kref f_ref;
@@ -571,7 +574,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;
@@ -603,13 +606,6 @@ 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) {
-			SG_LOG(1, pr_info("%s: command length too long\n",
-			       __func__));
-			sfp->next_cmd_len = 0;
-			sg_remove_request(sfp, srp);
-			return -EIO;
-		}
 		cmd_size = sfp->next_cmd_len;
 		sfp->next_cmd_len = 0;	/* reset so only this write() effected */
 	} else {
@@ -681,7 +677,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;
 
@@ -1657,13 +1653,23 @@ 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;
 
 	SG_LOG(4, pr_info("%s: dxfer_len=%d\n", __func__, 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) {
+		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;
@@ -1750,6 +1756,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 +24 -67

108 lines not on this page · raw message

maintainer
#1 lore raw
Quoted 10 lines · Douglas Gilbert

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.
- remove some dead code caused by this change
- cleanup comment block at the top of sg.h, fix urls

This doesn't apply:

patching file drivers/scsi/sg.c
Hunk #1 succeeded at 74 (offset 1 line).
Hunk #2 succeeded at 164 (offset 2 lines).
Hunk #3 succeeded at 569 (offset -5 lines).
Hunk #4 FAILED at 606.
Hunk #5 succeeded at 678 (offset -6 lines).
Hunk #6 FAILED at 1660.
Hunk #7 succeeded at 1742 (offset -11 lines).
2 out of 7 hunks FAILED -- saving rejects to file drivers/scsi/sg.c.rej

It looks like there's a missing intermediate patch changing the way logging is done within the driver ... I don't recall seeing such a patch on the list.

James

author maintainer
#2 lore raw
Quoted 24 lines · James Bottomley
Quoted 10 lines · Douglas Gilbert

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.
- remove some dead code caused by this change
- cleanup comment block at the top of sg.h, fix urls

This doesn't apply:

patching file drivers/scsi/sg.c
Hunk #1 succeeded at 74 (offset 1 line).
Hunk #2 succeeded at 164 (offset 2 lines).
Hunk #3 succeeded at 569 (offset -5 lines).
Hunk #4 FAILED at 606.
Hunk #5 succeeded at 678 (offset -6 lines).
Hunk #6 FAILED at 1660.
Hunk #7 succeeded at 1742 (offset -11 lines).
2 out of 7 hunks FAILED -- saving rejects to file drivers/scsi/sg.c.rej

Quoted 3 lines

It looks like there's a missing intermediate patch changing the way logging is done within the driver ... I don't recall seeing such a patch on the list.

It based on this patch:

    http://www.spinics.net/lists/linux-scsi/msg69957.html

sent to the list on 12 November titled:
[PATCH v3] sg: O_EXCL and other lock handling

and it was followed in the same day by:
[PATCH v3] sg: relax 16 byte cdb restriction
which you commented on, hence "v4" that started this thread.

The vagueness in the "O_EXCL" post about the reported failure in the vicinity of sg_remove() was cleared up a few days later by this post:

    http://www.spinics.net/lists/linux-scsi/msg70006.html

You might check if the fix to the st driver for this matter has made it into your tree or further.

I'm told the "O_EXCL" patch has been tested by a major vendor with help from another frequent correspondent to this list.

Doug Gilbert