[PATCH 1/2] SCSI tape fixes (new version): sense descriptor init, bsf->weof, blkno, debugging

[PATCH 1/2] SCSI tape fixes (new version): sense descriptor init, bsf->weof, blkno, debugging

[Kai Makisara]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7661 bytes --]

This is an updated version of the patch I sent March 7. The sense descriptor
initialization has been made lighter.

The patch at the end of this message applies to 2.6.11-bk7 + st descriptor sense
patch + st auto eof patch (i.e., st patches currently in scsi-misc-2.6). The
patch fixes the following problems:

- the sense descriptor fields are properly initialized
- BSF and BSFM are added to the commands causing automatic writing of filemark
  if the previous operation was write (tar expects this)
- the block number is set to unknown (-1) if spacing forward ends at BLANK CHECK
- debugging printout of spacing counts fixed to work also with 64-bit systems

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>

--- linux-2.6.11-bk7-k1/drivers/scsi/st.c	2005-03-12 13:38:32.000000000 +0200
+++ linux-2.6.11-bk7-k2/drivers/scsi/st.c	2005-03-12 13:52:30.000000000 +0200
@@ -17,7 +17,7 @@
    Last modified: 18-JAN-1998 Richard Gooch <rgooch@atnf.csiro.au> Devfs support
  */
 
-static char *verstr = "20050213";
+static char *verstr = "20050312";
 
 #include <linux/module.h>
 
@@ -269,8 +269,10 @@ static void st_analyze_sense(struct scsi
 	const u8 *sense = SRpnt->sr_sense_buffer;
 
 	s->have_sense = scsi_request_normalize_sense(SRpnt, &s->sense_hdr);
+	s->flags = 0;
 
 	if (s->have_sense) {
+		s->deferred = 0;
 		s->remainder_valid =
 			scsi_get_sense_info_fld(sense, SCSI_SENSE_BUFFERSIZE, &s->uremainder64);
 		switch (sense[0] & 0x7f) {
@@ -287,8 +289,6 @@ static void st_analyze_sense(struct scsi
 			ucp = scsi_sense_desc_find(sense, SCSI_SENSE_BUFFERSIZE, 4);
 			s->flags = ucp ? (ucp[3] & 0xe0) : 0;
 			break;
-		default:
-			s->flags = 0;
 		}
 	}
 }
@@ -306,7 +306,7 @@ static int st_chk_result(struct scsi_tap
 	if (!result)
 		return 0;
 
-		cmdstatp = &STp->buffer->cmdstat;
+	cmdstatp = &STp->buffer->cmdstat;
 	st_analyze_sense(STp->buffer->last_SRpnt, cmdstatp);
 
 	if (cmdstatp->have_sense)
@@ -2413,6 +2413,22 @@ static int do_load_unload(struct scsi_ta
 	return retval;
 }
 \f

+#if DEBUG
+#define ST_DEB_FORWARD  0
+#define ST_DEB_BACKWARD 1
+static void deb_space_print(char *name, int direction, char *units, unsigned char *cmd)
+{
+	s32 sc;
+
+	sc = cmd[2] & 0x80 ? 0xff000000 : 0;
+	sc |= (cmd[2] << 16) | (cmd[3] << 8) | cmd[4];
+	if (direction)
+		sc = -sc;
+	printk(ST_DEB_MSG "%s: Spacing tape %s over %d %s.\n", name,
+	       direction ? "backward" : "forward", sc, units);
+}
+#endif
+
 
 /* Internal ioctl function */
 static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned long arg)
@@ -2451,8 +2467,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[2] = (arg >> 16);
 		cmd[3] = (arg >> 8);
 		cmd[4] = arg;
-                DEBC(printk(ST_DEB_MSG "%s: Spacing tape forward over %d filemarks.\n",
-			    name, cmd[2] * 65536 + cmd[3] * 256 + cmd[4]));
+                DEBC(deb_space_print(name, ST_DEB_FORWARD, "filemarks", cmd);)
 		if (fileno >= 0)
 			fileno += arg;
 		blkno = 0;
@@ -2467,14 +2482,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[2] = (ltmp >> 16);
 		cmd[3] = (ltmp >> 8);
 		cmd[4] = ltmp;
-                DEBC(
-                     if (cmd[2] & 0x80)
-                     	ltmp = 0xff000000;
-                     ltmp = ltmp | (cmd[2] << 16) | (cmd[3] << 8) | cmd[4];
-                     printk(ST_DEB_MSG
-                            "%s: Spacing tape backward over %ld filemarks.\n",
-                            name, (-ltmp));
-		)
+                DEBC(deb_space_print(name, ST_DEB_BACKWARD, "filemarks", cmd);)
 		if (fileno >= 0)
 			fileno -= arg;
 		blkno = (-1);	/* We can't know the block number */
@@ -2486,8 +2494,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[2] = (arg >> 16);
 		cmd[3] = (arg >> 8);
 		cmd[4] = arg;
-                DEBC(printk(ST_DEB_MSG "%s: Spacing tape forward %d blocks.\n", name,
-			       cmd[2] * 65536 + cmd[3] * 256 + cmd[4]));
+                DEBC(deb_space_print(name, ST_DEB_FORWARD, "blocks", cmd);)
 		if (blkno >= 0)
 			blkno += arg;
 		at_sm &= (arg == 0);
@@ -2499,13 +2506,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[2] = (ltmp >> 16);
 		cmd[3] = (ltmp >> 8);
 		cmd[4] = ltmp;
-                DEBC(
-                     if (cmd[2] & 0x80)
-                          ltmp = 0xff000000;
-                     ltmp = ltmp | (cmd[2] << 16) | (cmd[3] << 8) | cmd[4];
-                     printk(ST_DEB_MSG
-                            "%s: Spacing tape backward %ld blocks.\n", name, (-ltmp));
-		)
+                DEBC(deb_space_print(name, ST_DEB_BACKWARD, "blocks", cmd);)
 		if (blkno >= 0)
 			blkno -= arg;
 		at_sm &= (arg == 0);
@@ -2516,8 +2517,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[2] = (arg >> 16);
 		cmd[3] = (arg >> 8);
 		cmd[4] = arg;
-                DEBC(printk(ST_DEB_MSG "%s: Spacing tape forward %d setmarks.\n", name,
-                            cmd[2] * 65536 + cmd[3] * 256 + cmd[4]));
+                DEBC(deb_space_print(name, ST_DEB_FORWARD, "setmarks", cmd);)
 		if (arg != 0) {
 			blkno = fileno = (-1);
 			at_sm = 1;
@@ -2530,13 +2530,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[2] = (ltmp >> 16);
 		cmd[3] = (ltmp >> 8);
 		cmd[4] = ltmp;
-                DEBC(
-                     if (cmd[2] & 0x80)
-				ltmp = 0xff000000;
-                     ltmp = ltmp | (cmd[2] << 16) | (cmd[3] << 8) | cmd[4];
-                     printk(ST_DEB_MSG "%s: Spacing tape backward %ld setmarks.\n",
-                            name, (-ltmp));
-		)
+                DEBC(deb_space_print(name, ST_DEB_BACKWARD, "setmarks", cmd);)
 		if (arg != 0) {
 			blkno = fileno = (-1);
 			at_sm = 1;
@@ -2606,7 +2600,7 @@ static int st_int_ioctl(struct scsi_tape
 		cmd[1] = 3;
                 DEBC(printk(ST_DEB_MSG "%s: Spacing to end of recorded medium.\n",
                             name));
-		blkno = 0;
+		blkno = -1;
 		at_sm = 0;
 		break;
 	case MTERASE:
@@ -2767,7 +2761,7 @@ static int st_int_ioctl(struct scsi_tape
 				STps->drv_file = fileno - undone;
 			else
 				STps->drv_file = fileno;
-			STps->drv_block = 0;
+			STps->drv_block = -1;
 			STps->eof = ST_NOEOF;
 		} else if ((cmd_in == MTBSF) || (cmd_in == MTBSFM)) {
 			if (arg > 0 && undone < 0)  /* Some drives get this wrong */
@@ -3245,12 +3239,15 @@ static int st_ioctl(struct inode *inode,
 			}
 			if (STps->rw == ST_WRITING &&
 			    (mtc.mt_op == MTREW || mtc.mt_op == MTOFFL ||
-			     mtc.mt_op == MTSEEK)) {
+			     mtc.mt_op == MTSEEK ||
+			     mtc.mt_op == MTBSF || mtc.mt_op == MTBSFM)) {
 				i = st_int_ioctl(STp, MTWEOF, 1);
 				if (i < 0) {
 					retval = i;
 					goto out;
 				}
+				if (mtc.mt_op == MTBSF || mtc.mt_op == MTBSFM)
+					mtc.mt_count++;
 				STps->rw = ST_IDLE;
 			     }
 
--- linux-2.6.11-bk7-k1/Documentation/scsi/st.txt	2005-03-07 19:38:45.000000000 +0200
+++ linux-2.6.11-bk7-k2/Documentation/scsi/st.txt	2005-03-07 21:14:44.000000000 +0200
@@ -2,7 +2,7 @@ This file contains brief information abo
 The driver is currently maintained by Kai Mäkisara (email
 Kai.Makisara@kolumbus.fi)
 
-Last modified: Sun Feb 20 22:28:27 2005 by kai.makisara
+Last modified: Mon Mar  7 21:14:44 2005 by kai.makisara
 
 
 BASICS
@@ -85,8 +85,8 @@ writing and the last operation has been 
 optionally written. In both cases end of data is signified by
 returning zero bytes for two consecutive reads.
 
-If the rewind, offline, or seek is done and previous tape operation was write,
-a filemark is written before moving tape.
+If rewind, offline, bsf, or seek is done and previous tape operation was
+write, a filemark is written before moving tape.
 
 The compile options are defined in the file linux/drivers/scsi/st_options.h.
 

[PATCH 2/2] SCSI tape fixes: remove f_pos handling

[Kai Makisara]

This patch applies over the previous patch in this thread. The patch removes
updating filp->f_pos. It has been dead code since 2.6.8 and nobody has missed
it.

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>

--- linux-2.6.11-bk7-k2/drivers/scsi/st.c	2005-03-12 13:52:30.000000000 +0200
+++ linux-2.6.11-bk7-k3/drivers/scsi/st.c	2005-03-12 18:11:11.000000000 +0200
@@ -1156,9 +1156,7 @@ static int st_flush(struct file *filp)
 	if (STps->rw == ST_WRITING && !STp->pos_unknown) {
 		struct st_cmdstatus *cmdstatp = &STp->buffer->cmdstat;
 
-                DEBC(printk(ST_DEB_MSG "%s: File length %lld bytes.\n",
-                            name, (long long)filp->f_pos);
-                     printk(ST_DEB_MSG "%s: Async write waits %d, finished %d.\n",
+                DEBC(printk(ST_DEB_MSG "%s: Async write waits %d, finished %d.\n",
                             name, STp->nbr_waits, STp->nbr_finished);
 		)
 
@@ -1520,7 +1518,6 @@ st_write(struct file *filp, const char _
 			}
 		}
 		count -= do_count;
-		filp->f_pos += do_count;
 		b_point += do_count;
 
 		async_write = STp->block_size == 0 && !STbp->do_dio &&
@@ -1580,7 +1577,6 @@ st_write(struct file *filp, const char _
 					undone = 0;
 				if (STp->block_size != 0)
 					undone *= STp->block_size;
-				filp->f_pos -= undone;
 				if (undone <= do_count) {
 					/* Only data from this write is not written */
 					count += undone;
@@ -1629,7 +1625,6 @@ st_write(struct file *filp, const char _
 					}
 				}
 			} else {
-				filp->f_pos -= do_count;
 				count += do_count;
 				STps->drv_block = (-1);		/* Too cautious? */
 				retval = (-EIO);
@@ -1958,7 +1953,6 @@ st_read(struct file *filp, char __user *
 					goto out;
 				}
 			}
-			filp->f_pos += transfer;
 			buf += transfer;
 			total += transfer;
 		}

[PATCH 3/2] SCSI tape security: require CAP_ADMIN for SG_IO etc.

[Kai Makisara]

The patch at the end is against 2.6.11-bk7 + the two patches in this
thread but it applies with offset to nearly any recent kernel.

The kernel currently allows any user permitted to access the tape device file
to send the tape drive commands that may either make the tape drivers internal
state inconsistent or to change the drive parameters so that other users find
the drive to be unusable. This patch changes ioctl handling so that SG_IO,
SCSI_IOCTL_COMMAND, etc. require CAP_ADMIN. This solves the consistency
problems for SCSI tapes. The st driver provides user-accessible commands to
change the drive parameters that users may need to access.

The SCSI command permissions were discussed widely on the linux lists but this
did not result in any useful refinement of the permissions. It may very well
be that the tape drives are the only devices that users are sometimes given
permissions to access and that have security problems with the current command
filtering. This patch solves the problem for tapes and no more elaborate
patches are needed.

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>

--- linux-2.6.11-bk7-k3/drivers/scsi/st.c	2005-03-12 18:11:11.000000000 +0200
+++ linux-2.6.11-bk7-k4/drivers/scsi/st.c	2005-03-13 19:24:29.000000000 +0200
@@ -3461,11 +3461,17 @@ static int st_ioctl(struct inode *inode,
 		case SCSI_IOCTL_GET_BUS_NUMBER:
 			break;
 		default:
-			i = scsi_cmd_ioctl(file, STp->disk, cmd_in, p);
+			if (!capable(CAP_SYS_ADMIN))
+				i = -EPERM;
+			else
+				i = scsi_cmd_ioctl(file, STp->disk, cmd_in, p);
 			if (i != -ENOTTY)
 				return i;
 			break;
 	}
+	if (!capable(CAP_SYS_ADMIN) &&
+	    (cmd_in == SCSI_IOCTL_START_UNIT || cmd_in == SCSI_IOCTL_STOP_UNIT))
+		return -EPERM;
 	return scsi_ioctl(STp->device, cmd_in, p);
 
  out:

Re: [PATCH 1/2] SCSI tape fixes (new version): sense descriptor init, bsf->weof, blkno, debugging

[James Bottomley]

On Sat, 2005-03-12 at 19:04 +0200, Kai Makisara wrote:
> This is an updated version of the patch I sent March 7. The sense descriptor
> initialization has been made lighter.
> 
> The patch at the end of this message applies to 2.6.11-bk7 + st descriptor sense
> patch + st auto eof patch (i.e., st patches currently in scsi-misc-2.6). The
> patch fixes the following problems:

I'm afraid this doesn't apply.  It looks like I've got an interim patch
that's crept in somewhere in the tree.

Could you take a look at scsi-misc-2.6 (or the patches on www.parisc-
linux/~jejb/scsi_diffs) and see if you can construct a diff between
where I currently am and where I should be?

Thanks,

James

Re: [PATCH 1/2] SCSI tape fixes (new version): sense descriptor init, bsf->weof, blkno, debugging

[Kai Makisara]

On Tue, 15 Mar 2005, James Bottomley wrote:

> On Sat, 2005-03-12 at 19:04 +0200, Kai Makisara wrote:
> > This is an updated version of the patch I sent March 7. The sense descriptor
> > initialization has been made lighter.
> > 
> > The patch at the end of this message applies to 2.6.11-bk7 + st descriptor sense
> > patch + st auto eof patch (i.e., st patches currently in scsi-misc-2.6). The
> > patch fixes the following problems:
> 
> I'm afraid this doesn't apply.  It looks like I've got an interim patch
> that's crept in somewhere in the tree.
> 
> Could you take a look at scsi-misc-2.6 (or the patches on www.parisc-
> linux/~jejb/scsi_diffs) and see if you can construct a diff between
> where I currently am and where I should be?
> 
The changelog shows that the patch below is included. It is the previous 
version of the patch in the message with this subject.

# ChangeSet
#   2005/03/15 10:37:07-06:00 Kai.Makisara@kolumbus.fi 
#   [PATCH] SCSI tape fixes: sense descriptor init, bsf->weof, blkno,

The patch at the end of this message should take you into the point where 
you can directly apply 2/2 etc. (If you think that the patch below is not 
worth the trouble, you can skip that. The other patches are not touching 
verstr and should then apply with small offset.)

Thanks,
Kai

This patch changes the sense descriptor initialization a little faster.

Signed-off-by: Kai Makisara <kai.makisara@kolumbus.fi>

--- linux-2.6.11-kk1/drivers/scsi/st.c	2005-03-15 20:55:12.000000000 +0200
+++ linux-2.6.11-bk7-k2/drivers/scsi/st.c	2005-03-12 13:52:30.000000000 +0200
@@ -17,7 +17,7 @@
    Last modified: 18-JAN-1998 Richard Gooch <rgooch@atnf.csiro.au> Devfs support
  */
 
-static char *verstr = "20050307";
+static char *verstr = "20050312";
 
 #include <linux/module.h>
 
@@ -268,10 +268,11 @@ static void st_analyze_sense(struct scsi
 	const u8 *ucp;
 	const u8 *sense = SRpnt->sr_sense_buffer;
 
-	memset(s, 0, sizeof(struct st_cmdstatus));
 	s->have_sense = scsi_request_normalize_sense(SRpnt, &s->sense_hdr);
+	s->flags = 0;
 
 	if (s->have_sense) {
+		s->deferred = 0;
 		s->remainder_valid =
 			scsi_get_sense_info_fld(sense, SCSI_SENSE_BUFFERSIZE, &s->uremainder64);
 		switch (sense[0] & 0x7f) {