Skip to contentSkip to conversation

drivers/block/scsi_ioctl problem

DISCUSSION Brian King 3 participants 9 messages Dec 14, 2004 – Oct 7, 2005

Subsystems (2)
SCSI Block layer
lore
Share & tools
lore

Brian King

author
#0 lore raw

Can someone explain to me the need for this bit of code in drivers/block/scsi_ioctl.c:

if (!(type & CMD_WARNED)) {
	cmd_type[cmd[0]] = CMD_WARNED;
	printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
}

I'm trying to convert an application to use SG_IO to /dev/sd* devices rather than using /dev/sg* devices and this is one problem I have been running into. Any time I issue a vendor specific scsi opcode I end up getting one of these error logs. Is there a good reason this error log is needed?

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
#1 lore raw
Quoted 8 lines · Brian King

Can someone explain to me the need for this bit of code in drivers/block/scsi_ioctl.c:

if (!(type & CMD_WARNED)) {
    cmd_type[cmd[0]] = CMD_WARNED;
    printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
}

I'm trying to convert an application to use SG_IO to /dev/sd* devices rather than using /dev/sg* devices and this is one problem I have been running into. Any time I issue a vendor specific scsi opcode I end up getting one of these error logs. Is there a good reason this error log is needed?

Brian,
I think command filtering was retrofitted into the block SG_IO code. You only need read permissions to execute an ioctl and FORMAT could be unfortunate.

IMO this code snippet should be moved from below the snippet you showed to the top of the procedural lines in that function:

        /* And root can do any command.. */
        if (capable(CAP_SYS_RAWIO))
                return 0;

That command filtering seems pretty haphazard, aimed mainly at cd/dvd devices. The START STOP command is regarded as "safe for read"** while SERVICE ACTION IN (12), (16), MAINTENANCE IN and PERSISTENT RESERVE IN are _not_ considered "safe for read".

** the sd driver doesn't react well when some other mechanism explicity spins down a dsik.

Doug Gilbert

#2 lore raw
In reply to Brian King
Quoted 8 lines · Brian King

Can someone explain to me the need for this bit of code in drivers/block/scsi_ioctl.c:

if (!(type & CMD_WARNED)) {
	cmd_type[cmd[0]] = CMD_WARNED;
	printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
}
Quoted 5 lines

I'm trying to convert an application to use SG_IO to /dev/sd* devices rather than using /dev/sg* devices and this is one problem I have been running into. Any time I issue a vendor specific scsi opcode I end up getting one of these error logs. Is there a good reason this error log is needed?

It was added to be able to find out which opcode was rejected and thus caused an application malfunction. It dumps the specific opcode only once, is that such a huge problem?

Signature · 1 line
Jens Axboe
author
#3 lore raw
Quoted 9 lines · Jens Axboe
Quoted 7 lines · Brian King

Can someone explain to me the need for this bit of code in drivers/block/scsi_ioctl.c:

if (!(type & CMD_WARNED)) {
	cmd_type[cmd[0]] = CMD_WARNED;
	printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
}
Quoted 6 lines
Quoted 1 line

I'm trying to convert an application to use SG_IO to /dev/sd* devices rather than using /dev/sg* devices and this is one problem I have been running into. Any time I issue a vendor specific scsi opcode I end up getting one of these error logs. Is there a good reason this error log is needed?

It was added to be able to find out which opcode was rejected and thus caused an application malfunction. It dumps the specific opcode only once, is that such a huge problem?

Ok. I didn't realize that it was only dumped once. That's not as bad. I guess it might be better if it were only dumped if the op actually failed.

Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
author
#4 lore raw
Quoted 6 lines · Brian King
Quoted 3 lines

It was added to be able to find out which opcode was rejected and thus caused an application malfunction. It dumps the specific opcode only once, is that such a huge problem?

Ok. I didn't realize that it was only dumped once. That's not as bad. I guess it might be better if it were only dumped if the op actually failed.

How about something like this...

Currently if an SG_IO ioctl is issued to a block device with an unknown opcode, an error is logged. This error is logged regardless the device successfully processes the command or not. This results in an error getting logged for each unknown opcode ever issued. This patch changes this policy and only prints the unknown opcode if the command fails.

Signed-off-by Brian King <[email protected]>

---

 linux-2.6.10-rc3-bk8-bjking1/drivers/block/scsi_ioctl.c |   19 +++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)
drivers/block/scsi_ioctl.c renamed from drivers/block/scsi_ioctl.c~blk_scsi_ioctl_unknown_cmd +16 -3
diff -puN drivers/block/scsi_ioctl.c~blk_scsi_ioctl_unknown_cmd drivers/block/scsi_ioctl.c
@@ -111,7 +111,7 @@ static int sg_emulated_host(request_queu
 #define safe_for_read(cmd)	[cmd] = CMD_READ_SAFE
 #define safe_for_write(cmd)	[cmd] = CMD_WRITE_SAFE
 
-static int verify_command(struct file *file, unsigned char *cmd)
+static int __verify_command(struct file *file, unsigned char *cmd, int logging)
 {
 	static unsigned char cmd_type[256] = {
 

@@ -199,7 +199,7 @@ static int verify_command(struct file *f
 			return 0;
 	}
 
-	if (!(type & CMD_WARNED)) {
+	if (logging && !(type & CMD_WARNED)) {
 		cmd_type[cmd[0]] = CMD_WARNED;
 		printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
 	}

@@ -212,6 +212,16 @@ static int verify_command(struct file *f
 	return -EPERM;
 }
 
+static int verify_command(struct file *file, unsigned char *cmd)
+{
+	return __verify_command(file, cmd, 0);
+}
+
+static void log_unknown_opcode(struct file *file, unsigned char *cmd)
+{
+	__verify_command(file, cmd, 1);
+}
+
 static int sg_io(struct file *file, request_queue_t *q,
 		struct gendisk *bd_disk, struct sg_io_hdr *hdr)
 {

@@ -307,8 +317,10 @@ static int sg_io(struct file *file, requ
 	hdr->host_status = host_byte(rq->errors);
 	hdr->driver_status = driver_byte(rq->errors);
 	hdr->info = 0;
-	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
+	if (hdr->masked_status || hdr->host_status || hdr->driver_status) {
 		hdr->info |= SG_INFO_CHECK;
+		log_unknown_opcode(file, cmd);
+	}
 	hdr->resid = rq->data_len;
 	hdr->duration = ((jiffies - start_time) * 1000) / HZ;
 	hdr->sb_len_wr = 0;

@@ -415,6 +427,7 @@ static int sg_scsi_ioctl(struct file *fi
 	blk_execute_rq(q, bd_disk, rq);
 	err = rq->errors & 0xff;	/* only 8 bit SCSI status */
 	if (err) {
+		log_unknown_opcode(file, rq->cmd);
 		if (rq->sense_len && rq->sense) {
 			bytes = (OMAX_SB_LEN > rq->sense_len) ?
 				rq->sense_len : OMAX_SB_LEN;

_

#5 lore raw
Quoted 12 lines · Brian King
Quoted 7 lines · Brian King
Quoted 3 lines

It was added to be able to find out which opcode was rejected and thus caused an application malfunction. It dumps the specific opcode only once, is that such a huge problem?

Ok. I didn't realize that it was only dumped once. That's not as bad. I guess it might be better if it were only dumped if the op actually failed.

How about something like this...

Quoted 6 lines

Currently if an SG_IO ioctl is issued to a block device with an unknown opcode, an error is logged. This error is logged regardless the device successfully processes the command or not. This results in an error getting logged for each unknown opcode ever issued. This patch changes this policy and only prints the unknown opcode if the command fails.

How about just moving it after the CAP_SYS_RAWIO check, so that it only logs for commands we explicitly reject? The purpose of the opcode dump is not to log failed commands (the app should do that itself), but mainly why a specific opcode was _not_ sent to the drive. This is something you cannot always directly gauge, unless you have the source for the app. And even then you have to dig :-)

===== drivers/block/scsi_ioctl.c 1.62 vs edited =====

drivers/block/scsi_ioctl.c +4 -4
@@ -199,14 +199,14 @@
 			return 0;
 	}
 
+	/* And root can do any command.. */
+	if (capable(CAP_SYS_RAWIO))
+		return 0;
+
 	if (!(type & CMD_WARNED)) {
 		cmd_type[cmd[0]] = CMD_WARNED;
 		printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
 	}
-
-	/* And root can do any command.. */
-	if (capable(CAP_SYS_RAWIO))
-		return 0;
 
 	/* Otherwise fail it with an "Operation not permitted" */
 	return -EPERM;
Signature · 1 line
Jens Axboe
author
#6 lore raw
Quoted 22 lines · Jens Axboe
Quoted 12 lines · Brian King
Quoted 6 lines · Brian King
Quoted 3 lines

It was added to be able to find out which opcode was rejected and thus caused an application malfunction. It dumps the specific opcode only once, is that such a huge problem?

Ok. I didn't realize that it was only dumped once. That's not as bad. I guess it might be better if it were only dumped if the op actually failed.

How about something like this...

Quoted 6 lines

Currently if an SG_IO ioctl is issued to a block device with an unknown opcode, an error is logged. This error is logged regardless the device successfully processes the command or not. This results in an error getting logged for each unknown opcode ever issued. This patch changes this policy and only prints the unknown opcode if the command fails.

Quoted 6 lines

How about just moving it after the CAP_SYS_RAWIO check, so that it only logs for commands we explicitly reject? The purpose of the opcode dump is not to log failed commands (the app should do that itself), but mainly why a specific opcode was _not_ sent to the drive. This is something you cannot always directly gauge, unless you have the source for the app. And even then you have to dig :-)

I guess that would make more sense. Patch looks good.

Thanks

-Brian

Quoted 25 lines

===== drivers/block/scsi_ioctl.c 1.62 vs edited =====

drivers/block/scsi_ioctl.c +4 -4
@@ -199,14 +199,14 @@
 			return 0;
 	}
 
+	/* And root can do any command.. */
+	if (capable(CAP_SYS_RAWIO))
+		return 0;
+
 	if (!(type & CMD_WARNED)) {
 		cmd_type[cmd[0]] = CMD_WARNED;
 		printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
 	}
-
-	/* And root can do any command.. */
-	if (capable(CAP_SYS_RAWIO))
-		return 0;
 
 	/* Otherwise fail it with an "Operation not permitted" */
 	return -EPERM;
Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
author
#7 lore raw
In reply to Jens Axboe

Jens,

Looks like this patch got dropped for some reason.

Brian

Quoted 54 lines · Jens Axboe
Quoted 13 lines · Brian King
Quoted 7 lines · Brian King
Quoted 3 lines

It was added to be able to find out which opcode was rejected and thus caused an application malfunction. It dumps the specific opcode only once, is that such a huge problem?

Ok. I didn't realize that it was only dumped once. That's not as bad. I guess it might be better if it were only dumped if the op actually failed.

How about something like this...

Quoted 6 lines

Currently if an SG_IO ioctl is issued to a block device with an unknown opcode, an error is logged. This error is logged regardless the device successfully processes the command or not. This results in an error getting logged for each unknown opcode ever issued. This patch changes this policy and only prints the unknown opcode if the command fails.

How about just moving it after the CAP_SYS_RAWIO check, so that it only logs for commands we explicitly reject? The purpose of the opcode dump is not to log failed commands (the app should do that itself), but mainly why a specific opcode was _not_ sent to the drive. This is something you cannot always directly gauge, unless you have the source for the app. And even then you have to dig :-)

===== drivers/block/scsi_ioctl.c 1.62 vs edited =====

drivers/block/scsi_ioctl.c +4 -4
@@ -199,14 +199,14 @@
 			return 0;
 	}
 
+	/* And root can do any command.. */
+	if (capable(CAP_SYS_RAWIO))
+		return 0;
+
 	if (!(type & CMD_WARNED)) {
 		cmd_type[cmd[0]] = CMD_WARNED;
 		printk(KERN_WARNING "scsi: unknown opcode 0x%02x\n", cmd[0]);
 	}
-
-	/* And root can do any command.. */
-	if (capable(CAP_SYS_RAWIO))
-		return 0;
 
 	/* Otherwise fail it with an "Operation not permitted" */
 	return -EPERM;
Signature · 3 lines
Brian King
eServer Storage I/O
IBM Linux Technology Center
#8 lore raw
Quoted 3 lines · Brian King

Jens,

Looks like this patch got dropped for some reason.

Indeed, I resubmitted it now.

Signature · 1 line
Jens Axboe