Oops in usb-storage.c

Quoted 1 line

EIP; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====

Hi,

usb-storage.c oopses in fill_inquiry_response if I send an INQUIRY to device which is currently disconnected from the USB bus.

This happens because fill_inquiry_response is called outside a check for us->pusb_dev. Moving the special case into the if() block, the oops is fixed.

(For reference, the oops is below the patch)

Jan

drivers/usb/storage/usb.c renamed from drivers/usb/storage/usb.c.orig +18 -21

@@ -389,24 +389,6 @@
 				break;
 			}
 
-			/* Handle those devices which need us to fake their
-			 * inquiry data */
-			if ((us->srb->cmnd[0] == INQUIRY) &&
-			    (us->flags & US_FL_FIX_INQUIRY)) {
-			    	unsigned char data_ptr[36] = {
-				    0x00, 0x80, 0x02, 0x02,
-				    0x1F, 0x00, 0x00, 0x00};
-
-			    	US_DEBUGP("Faking INQUIRY command\n");
-				fill_inquiry_response(us, data_ptr, 36);
-				us->srb->result = GOOD << 1;
-
-				set_current_state(TASK_INTERRUPTIBLE);
-				us->srb->scsi_done(us->srb);
-				us->srb = NULL;
-				break;
-			}
-
 			/* lock the device pointers */
 			down(&(us->dev_semaphore));
 

@@ -423,15 +405,30 @@
 					       sizeof(usb_stor_sense_notready));
 					us->srb->result = GOOD << 1;
 				} else {
+					memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
 					memcpy(us->srb->sense_buffer, 
 					       usb_stor_sense_notready, 
 					       sizeof(usb_stor_sense_notready));
 					us->srb->result = CHECK_CONDITION << 1;
 				}
 			} else { /* !us->pusb_dev */
-				/* we've got a command, let's do it! */
-				US_DEBUG(usb_stor_show_command(us->srb));
-				us->proto_handler(us->srb, us);
+
+				/* Handle those devices which need us to fake 
+				 * their inquiry data */
+				if ((us->srb->cmnd[0] == INQUIRY) &&
+				    (us->flags & US_FL_FIX_INQUIRY)) {
+					unsigned char data_ptr[36] = {
+					    0x00, 0x80, 0x02, 0x02,
+					    0x1F, 0x00, 0x00, 0x00};
+
+					US_DEBUGP("Faking INQUIRY command\n");
+					fill_inquiry_response(us, data_ptr, 36);
+					us->srb->result = GOOD << 1;
+				} else {
+					/* we've got a command, let's do it! */
+					US_DEBUG(usb_stor_show_command(us->srb));
+					us->proto_handler(us->srb, us);
+				}
 			}
 
 			/* unlock the device pointers */

Oct 16 21:07:28 sirith kernel: Oops: 0000
Oct 16 21:07:28 sirith kernel: CPU:    0
Oct 16 21:07:28 sirith kernel: EIP:    0010:[<e4951766>]    Tainted: P 
Oct 16 21:07:28 sirith kernel: EFLAGS: 00010246
Oct 16 21:07:28 sirith kernel: eax: 00000000   ebx: dc636600   ecx: 00000000   edx: 00000010
Oct 16 21:07:28 sirith kernel: esi: e495d460   edi: d9f09fcc   ebp: e495d450   esp: d9f09f7c
Oct 16 21:07:28 sirith kernel: ds: 0018   es: 0018   ss: 0018
Oct 16 21:07:28 sirith kernel: Process usb-storage-0 (pid: 766, stackpage=d9f09000)
Oct 16 21:07:28 sirith kernel: Stack: d9f08000 e495da91 d9f09ff0 dc636600 c0116373 c02955a7 00000005 c01162c4 
Oct 16 21:07:28 sirith kernel:        d9f08000 e4951b44 dc636600 d9f09fcc 00000024 e495daa0 00000100 da003dcc 
Oct 16 21:07:28 sirith kernel:        dc636600 dc636600 dc636604 00000001 02028000 0000001f 69736143 0000006f 
Oct 16 21:07:28 sirith kernel: Call Trace: [<e495da91>] [release_console_sem+115/128] [printk+260/272] [<e4951b44>] [<e495daa0>] 
Oct 16 21:07:28 sirith kernel: Code: 0f b7 80 cc 00 00 00 66 c1 e8 0c 0c 30 88 47 20 8b 43 18 8a 

Quoted 1 line

Quoted 1 line

EIP; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====

Trace; e495da90 <[usb-storage]__module_usb_device_size+670/81be> Code; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> 00000000 <_EIP>:
Code; e4951766 <[usb-storage]fill_inquiry_response+116/2f0> <=====
0: 0f b7 80 cc 00 00 00 movzwl 0xcc(%eax),%eax <=====
Code; e495176c <[usb-storage]fill_inquiry_response+11c/2f0>
7: 66 c1 e8 0c shr $0xc,%ax
Code; e4951770 <[usb-storage]fill_inquiry_response+120/2f0>
b: 0c 30 or $0x30,%al
Code; e4951772 <[usb-storage]fill_inquiry_response+122/2f0>
d: 88 47 20 mov %al,0x20(%edi)
Code; e4951776 <[usb-storage]fill_inquiry_response+126/2f0>
10: 8b 43 18 mov 0x18(%ebx),%eax
Code; e4951778 <[usb-storage]fill_inquiry_response+128/2f0>
13: 8a 00 mov (%eax),%al

Actually, this is a side-effect of another problem, which is that INQUIRY is legal for a device at any time (at least, to SCSI). What we really need to do is fake an INQURIY response for detached devices, separate from also those devices which need a faked-inquiry all the time.

Quoted 4 lines · Matthew Dharm

Actually, this is a side-effect of another problem, which is that INQUIRY is legal for a device at any time (at least, to SCSI). What we really need to do is fake an INQURIY response for detached devices, separate from also those devices which need a faked-inquiry all the time.

Ok, then the fix could look like the following, I think. The INQUIRY response in the disconnected case is a little bit different, as the information from pusb_dev is not available, but the INQUIRY works and the oops is fixed.

Jan

drivers/usb/storage/usb.c renamed from drivers/usb/storage/usb.c.orig +6 -5

@@ -268,10 +268,12 @@
 	memcpy(data+16, us->unusual_dev->productName, 
 		strlen(us->unusual_dev->productName) > 16 ? 16 :
 		strlen(us->unusual_dev->productName));
-	data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
-	data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
-	data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
-	data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+	if(us->pusb_dev) {
+		data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
+		data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
+		data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
+		data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+	}
 
 	if (us->srb->use_sg) {
 		sg = (struct scatterlist *)us->srb->request_buffer;
 
 
 
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines · Matthew Dharm

No, I think something different is best.

Your first change to push the US_FL_FIX_INQUIRY code down after the test for pusb_dev is good.. but, in the case where that pointer is bad, we need to cook up something totally fake, like INQUIRY data that says "DISCONNECTED" "USB-STORAGE DEVICE" or somesuch.....

Then I don't understand why we have to answer the INQUIRY at all. This could as well be a disconnected / turned off external scsi device. I often turn off my external scsi scanner while the system is running, without a problem up to now. The scanner surely doesn't answer to INQUIRYs when it's turned off...

By the way - a device not needing the US_FL_FIX_INQUIRY code wouldn't answer to INQUIRYs either, when it's disconnected. So the patch I sent first gives the same behaviour for devices with and without US_FL_FIX_INQUIRY set.

Jan

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Technically, as long as the system believes that the target exists (i.e. you haven't unloaded your SCSI driver module), the target is required to respond to an INQUIRY command. So, if you boot with the scanner on, and then turn it off, you've got a problem.

Quoted 4 lines · Matthew Dharm

Technically, as long as the system believes that the target exists (i.e. you haven't unloaded your SCSI driver module), the target is required to respond to an INQUIRY command. So, if you boot with the scanner on, and then turn it off, you've got a problem.

Ok. I looked at the SCSI-2 standard and found a possibly sensible answer for such an INQUIRY to a disconnected device.
First, there is a special peripheral qualifier for disconnected physical devices:

001b The target is capable of supporting the specified peripheral
device type on this logical unit; however, the physical device is not currently connected to this logical unit.

Second, the devices is not required to give a complete answer:

NOTES
65 The INQUIRY command is typically used by the initiator after a reset or power-up condition to determine the device types for system configuration. To minimize delays after a reset or power-up condition, the standard INQUIRY data should be available without incurring any media access delays. If the target does store some of the INQUIRY data on the device, it may return zeros or ASCII spaces (20h) in those fields until the data is available from the device.

While this permission to set some fields to zero is included in the standard for other purposes, it makes clear that you must expect to get incomplete answers from an INQUIRY command.

So, what about the following patch?

Jan

drivers/usb/storage/usb.c renamed from drivers/usb/storage/usb.c.orig +47 -32

@@ -262,16 +262,28 @@
 	if (data_len<36) // You lose.
 		return;
 
-	memcpy(data+8, us->unusual_dev->vendorName, 
-		strlen(us->unusual_dev->vendorName) > 8 ? 8 :
-		strlen(us->unusual_dev->vendorName));
-	memcpy(data+16, us->unusual_dev->productName, 
-		strlen(us->unusual_dev->productName) > 16 ? 16 :
-		strlen(us->unusual_dev->productName));
-	data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
-	data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
-	data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
-	data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+	if(data[0]&0x20) { /* USB device currently not connected. Return
+			      peripheral qualifier 001b ("...however, the
+			      physical device is not currently connected
+			      to this logical unit") and leave vendor and
+			      product identification empty. ("If the target
+			      does store some of the INQUIRY data on the
+			      device, it may return zeros or ASCII spaces 
+			      (20h) in those fields until the data is
+			      available from the device."). */
+		memset(data+8,0,28);
+	} else {
+		memcpy(data+8, us->unusual_dev->vendorName, 
+			strlen(us->unusual_dev->vendorName) > 8 ? 8 :
+			strlen(us->unusual_dev->vendorName));
+		memcpy(data+16, us->unusual_dev->productName, 
+			strlen(us->unusual_dev->productName) > 16 ? 16 :
+			strlen(us->unusual_dev->productName));
+		data[32] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>12) & 0x0F);
+		data[33] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>8) & 0x0F);
+		data[34] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice>>4) & 0x0F);
+		data[35] = 0x30 + ((us->pusb_dev->descriptor.bcdDevice) & 0x0F);
+	}
 
 	if (us->srb->use_sg) {
 		sg = (struct scatterlist *)us->srb->request_buffer;

@@ -389,24 +401,6 @@
 				break;
 			}
 
-			/* Handle those devices which need us to fake their
-			 * inquiry data */
-			if ((us->srb->cmnd[0] == INQUIRY) &&
-			    (us->flags & US_FL_FIX_INQUIRY)) {
-			    	unsigned char data_ptr[36] = {
-				    0x00, 0x80, 0x02, 0x02,
-				    0x1F, 0x00, 0x00, 0x00};
-
-			    	US_DEBUGP("Faking INQUIRY command\n");
-				fill_inquiry_response(us, data_ptr, 36);
-				us->srb->result = GOOD << 1;
-
-				set_current_state(TASK_INTERRUPTIBLE);
-				us->srb->scsi_done(us->srb);
-				us->srb = NULL;
-				break;
-			}
-
 			/* lock the device pointers */
 			down(&(us->dev_semaphore));
 

@@ -422,16 +416,38 @@
 					       usb_stor_sense_notready, 
 					       sizeof(usb_stor_sense_notready));
 					us->srb->result = GOOD << 1;
+				} else if(us->srb->cmnd[0] == INQUIRY) {
+					unsigned char data_ptr[36] = {
+					    0x20, 0x80, 0x02, 0x02,
+					    0x1F, 0x00, 0x00, 0x00};
+					US_DEBUGP("Faking INQUIRY command for disconnected device\n");
+					fill_inquiry_response(us, data_ptr, 36);
+					us->srb->result = GOOD << 1;
 				} else {
+					memset(us->srb->request_buffer, 0, us->srb->request_bufflen);
 					memcpy(us->srb->sense_buffer, 
 					       usb_stor_sense_notready, 
 					       sizeof(usb_stor_sense_notready));
 					us->srb->result = CHECK_CONDITION << 1;
 				}
 			} else { /* !us->pusb_dev */
-				/* we've got a command, let's do it! */
-				US_DEBUG(usb_stor_show_command(us->srb));
-				us->proto_handler(us->srb, us);
+
+				/* Handle those devices which need us to fake 
+				 * their inquiry data */
+				if ((us->srb->cmnd[0] == INQUIRY) &&
+				    (us->flags & US_FL_FIX_INQUIRY)) {
+					unsigned char data_ptr[36] = {
+					    0x00, 0x80, 0x02, 0x02,
+					    0x1F, 0x00, 0x00, 0x00};
+
+					US_DEBUGP("Faking INQUIRY command\n");
+					fill_inquiry_response(us, data_ptr, 36);
+					us->srb->result = GOOD << 1;
+				} else {
+					/* we've got a command, let's do it! */
+					US_DEBUG(usb_stor_show_command(us->srb));
+					us->proto_handler(us->srb, us);
+				}
 			}
 
 			/* unlock the device pointers */
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

This looks better... but you have a dangerous memset() in an else clause -- the request buffer pointer could be pointing to a scatter-gather list, so you can't just memset on it.

This looks better... but you have a dangerous memset() in an else clause -- the request buffer pointer could be pointing to a scatter-gather list, so you can't just memset on it.