drivers/block/scsi_ioctl problem

drivers/block/scsi_ioctl problem

[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 King
eServer Storage I/O
IBM Linux Technology Center

Re: drivers/block/scsi_ioctl problem

[Douglas Gilbert]

Brian King wrote:
> 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

Re: drivers/block/scsi_ioctl problem

[Jens Axboe]

On Tue, Dec 14 2004, Brian King wrote:
> 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?

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?

-- 
Jens Axboe

Re: drivers/block/scsi_ioctl problem

[Brian King]

Jens Axboe wrote:
> On Tue, Dec 14 2004, Brian King wrote:
> 
>>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?
> 
> 
> 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.


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: drivers/block/scsi_ioctl problem

[Brian King]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

Brian King wrote:
>> 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...

[-- Attachment #2: blk_scsi_ioctl_unknown_cmd.patch --]
[-- Type: text/plain, Size: 2679 bytes --]


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 <brking@us.ibm.com>
---

 linux-2.6.10-rc3-bk8-bjking1/drivers/block/scsi_ioctl.c |   19 +++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff -puN drivers/block/scsi_ioctl.c~blk_scsi_ioctl_unknown_cmd drivers/block/scsi_ioctl.c
--- linux-2.6.10-rc3-bk8/drivers/block/scsi_ioctl.c~blk_scsi_ioctl_unknown_cmd	2004-12-15 14:19:34.000000000 -0600
+++ linux-2.6.10-rc3-bk8-bjking1/drivers/block/scsi_ioctl.c	2004-12-15 14:55:04.000000000 -0600
@@ -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;
_

Re: drivers/block/scsi_ioctl problem

[Jens Axboe]

On Wed, Dec 15 2004, Brian King wrote:
> Brian King wrote:
> >>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.

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 =====
--- 1.62/drivers/block/scsi_ioctl.c	2004-11-20 01:50:56 +01:00
+++ edited/drivers/block/scsi_ioctl.c	2004-12-16 07:30:39 +01:00
@@ -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;


-- 
Jens Axboe

Re: drivers/block/scsi_ioctl problem

[Brian King]

Jens Axboe wrote:
> On Wed, Dec 15 2004, Brian King wrote:
> 
>>Brian King wrote:
>>
>>>>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.
> 
> 
> 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

> 
> ===== drivers/block/scsi_ioctl.c 1.62 vs edited =====
> --- 1.62/drivers/block/scsi_ioctl.c	2004-11-20 01:50:56 +01:00
> +++ edited/drivers/block/scsi_ioctl.c	2004-12-16 07:30:39 +01:00
> @@ -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;
> 
> 


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: drivers/block/scsi_ioctl problem

[Brian King]

Jens,

Looks like this patch got dropped for some reason.

Brian

Jens Axboe wrote:
> On Wed, Dec 15 2004, Brian King wrote:
> 
>>Brian King wrote:
>>
>>>>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.
> 
> 
> 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 =====
> --- 1.62/drivers/block/scsi_ioctl.c	2004-11-20 01:50:56 +01:00
> +++ edited/drivers/block/scsi_ioctl.c	2004-12-16 07:30:39 +01:00
> @@ -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;
> 
> 


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: drivers/block/scsi_ioctl problem

[Jens Axboe]

On Fri, Oct 07 2005, Brian King wrote:
> Jens,
> 
> Looks like this patch got dropped for some reason.

Indeed, I resubmitted it now.

-- 
Jens Axboe