gdth: Allocate sense_buffer to prevent NULL pointer dereference

Hi,

i'm facing the following kernel oops with the latest git:

--------------------------------------8<-------------------------------------- Stopping MD arrays...failed (no MD subsystem loaded).
Mounting root filesystem read-only...done.
Will now restart.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
PGD 7e51e067 PUD 7e6a1067 PMD 0 
Oops: 0002 [1] PREEMPT SMP 
CPU 3 
Modules linked in: nvidia(P) lm85 hwmon_vid hwmon [last unloaded: nvidia]
Pid: 0, comm: swapper Tainted: P         2.6.25-rc4-smp-00134-g84c6f60 #11 
RIP: 0010:[<ffffffff8051a97e>]  [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
RSP: 0018:ffff81007fbefed8  EFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff81007e0dda68 RCX: 0000000000000002
RDX: 00000000ffffffff RSI: 0000000000000000 RDI: ffffc20000330000
RBP: ffff81007fbeff08 R08: 0000000000000000 R09: ffff81007f01de70
R10: 0000000000000000 R11: 0000000000000050 R12: ffff810000b10480
R13: ffff810000b104ff R14: ffff81007e214200 R15: 0000000000000009
FS:  0000000000000000(0000) GS:ffff81007f802c80(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 000000007e643000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffff81007fbe6000, task ffff81007fbe4000)
Stack:  0000000000000046 ffff81007e8a93c0 0000000000000000 0000000000000000
 0000000000000018 0000000000000003 ffff81007fbeff18 ffffffff8051ab9f
 ffff81007fbeff48 ffffffff80258cc2 ffffffff8092e300 ffff81007e8a93c0
Call Trace:
 <IRQ>  [<ffffffff8051ab9f>] gdth_interrupt+0x10/0x12
 [<ffffffff80258cc2>] handle_IRQ_event+0x27/0x57
 [<ffffffff8025a055>] handle_fasteoi_irq+0x9c/0xdc
 [<ffffffff8020def0>] do_IRQ+0x88/0xfc
 [<ffffffff80209ffe>] ? default_idle+0x0/0x5f
 [<ffffffff8020b851>] ret_from_intr+0x0/0xa
 <EOI>  [<ffffffff8020a037>] ? default_idle+0x39/0x5f
 [<ffffffff8020a032>] ? default_idle+0x34/0x5f
 [<ffffffff80209ffe>] ? default_idle+0x0/0x5f
 [<ffffffff8020a11c>] ? cpu_idle+0xbf/0xf5
 [<ffffffff806cd973>] ? start_secondary+0x3e0/0x3ef


Code: 00 04 eb 15 3c 17 75 11 41 0f b6 c5 48 c1 e0 05 41 80 a4 04 92 02 00 00 fb 41 c7 86 18 01 00 00 00 00 00 00 49 8b 86 c0 00 00 00 <c6> 00 00 e9 92 01 00 00 66 89 43 24 41 8b 84 24 68 02 00 00 89 
RIP  [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
 RSP <ffff81007fbefed8>
CR2: 0000000000000000
---[ end trace e81e561a458e8791 ]---
Kernel panic - not syncing: Aiee, killing interrupt handler!

Rebooting in 5 seconds.. --------------------------------------8<--------------------------------------

From 2dc63f9f8e61fd1c89f8b4d9b2d174be1c3bfbe2 Mon Sep 17 00:00:00 2001
From: root <[email protected]>
Date: Sun, 9 Mar 2008 13:25:07 +0100
Subject:

This fixes a NULL pointer dereference during execution of Internal commands, where gdth only allocates scp, but not scp->sense_buffer. The rest of the code assumes that sense_buffer is allocated, which leads to a kernel oops e.g. on reboot (during cache flush).

So we have two choices here:

a) Allocate the sense_buffer
b) surrounding all accesses to sense_buffer with some if (!internal_command)

I'm using solution a, as this keeps code simpler.

Signed-off-by Sven Schnelle <[email protected]>

---
 drivers/scsi/gdth.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/scsi/gdth.c +6 -0

index 27ebd33..0b2080d 100644
@@ -493,6 +493,12 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     if (!scp)
         return -ENOMEM;
 
+    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
+    if (!scp->sense_buffer) {
+	kfree(scp);
+	return -ENOMEM;
+    }
+
     scp->device = sdev;
     memset(&cmndinfo, 0, sizeof(cmndinfo));
 

Quoted 93 lines · Sven Schnelle

Hi,

i'm facing the following kernel oops with the latest git:

--------------------------------------8<-------------------------------------- Stopping MD arrays...failed (no MD subsystem loaded).
Mounting root filesystem read-only...done.
Will now restart.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
PGD 7e51e067 PUD 7e6a1067 PMD 0 
Oops: 0002 [1] PREEMPT SMP 
CPU 3 
Modules linked in: nvidia(P) lm85 hwmon_vid hwmon [last unloaded: nvidia]
Pid: 0, comm: swapper Tainted: P         2.6.25-rc4-smp-00134-g84c6f60 #11 
RIP: 0010:[<ffffffff8051a97e>]  [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
RSP: 0018:ffff81007fbefed8  EFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff81007e0dda68 RCX: 0000000000000002
RDX: 00000000ffffffff RSI: 0000000000000000 RDI: ffffc20000330000
RBP: ffff81007fbeff08 R08: 0000000000000000 R09: ffff81007f01de70
R10: 0000000000000000 R11: 0000000000000050 R12: ffff810000b10480
R13: ffff810000b104ff R14: ffff81007e214200 R15: 0000000000000009
FS:  0000000000000000(0000) GS:ffff81007f802c80(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 000000007e643000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffff81007fbe6000, task ffff81007fbe4000)
Stack:  0000000000000046 ffff81007e8a93c0 0000000000000000 0000000000000000
 0000000000000018 0000000000000003 ffff81007fbeff18 ffffffff8051ab9f
 ffff81007fbeff48 ffffffff80258cc2 ffffffff8092e300 ffff81007e8a93c0
Call Trace:
 <IRQ>  [<ffffffff8051ab9f>] gdth_interrupt+0x10/0x12
 [<ffffffff80258cc2>] handle_IRQ_event+0x27/0x57
 [<ffffffff8025a055>] handle_fasteoi_irq+0x9c/0xdc
 [<ffffffff8020def0>] do_IRQ+0x88/0xfc
 [<ffffffff80209ffe>] ? default_idle+0x0/0x5f
 [<ffffffff8020b851>] ret_from_intr+0x0/0xa
 <EOI>  [<ffffffff8020a037>] ? default_idle+0x39/0x5f
 [<ffffffff8020a032>] ? default_idle+0x34/0x5f
 [<ffffffff80209ffe>] ? default_idle+0x0/0x5f
 [<ffffffff8020a11c>] ? cpu_idle+0xbf/0xf5
 [<ffffffff806cd973>] ? start_secondary+0x3e0/0x3ef


Code: 00 04 eb 15 3c 17 75 11 41 0f b6 c5 48 c1 e0 05 41 80 a4 04 92 02 00 00 fb 41 c7 86 18 01 00 00 00 00 00 00 49 8b 86 c0 00 00 00 <c6> 00 00 e9 92 01 00 00 66 89 43 24 41 8b 84 24 68 02 00 00 89 
RIP  [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
 RSP <ffff81007fbefed8>
CR2: 0000000000000000
---[ end trace e81e561a458e8791 ]---
Kernel panic - not syncing: Aiee, killing interrupt handler!

Rebooting in 5 seconds.. --------------------------------------8<--------------------------------------

From 2dc63f9f8e61fd1c89f8b4d9b2d174be1c3bfbe2 Mon Sep 17 00:00:00 2001
From: root <[email protected]>
Date: Sun, 9 Mar 2008 13:25:07 +0100
Subject:

This fixes a NULL pointer dereference during execution of Internal commands, where gdth only allocates scp, but not scp->sense_buffer. The rest of the code assumes that sense_buffer is allocated, which leads to a kernel oops e.g. on reboot (during cache flush).

So we have two choices here:

a) Allocate the sense_buffer
b) surrounding all accesses to sense_buffer with some if (!internal_command)

I'm using solution a, as this keeps code simpler.

Signed-off-by Sven Schnelle <[email protected]>

---
 drivers/scsi/gdth.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/scsi/gdth.c +6 -0

index 27ebd33..0b2080d 100644
@@ -493,6 +493,12 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     if (!scp)
         return -ENOMEM;
 
+    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
+    if (!scp->sense_buffer) {
+	kfree(scp);
+	return -ENOMEM;
+    }
+
     scp->device = sdev;
     memset(&cmndinfo, 0, sizeof(cmndinfo));
 

drivers/scsi/gdth.c int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd, · line 525

Quoted 8 lines

drivers/scsi/gdth.c +1 -0

@@ -513,6 +519,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     rval = cmndinfo.status;
     if (info)
         *info = cmndinfo.info;
+    kfree(scp->sense_buffer);
     kfree(scp);
     return rval;
 }

James and linux-scsi CCed.

James it looks reasonable. It's a fallout from the sense_buffer separation patches.

Reviewed-by Boaz Harrosh <[email protected]>

I will submit solution b) above as part of my sense handling patchset.

Quoted 1 line · Boaz Harrosh

James and linux-scsi CCed.

Looks fine .. could someone send the patch in an applyable form (i.e. not quoted).

Quoted 4 lines · James Bottomley

Quoted 1 line · Boaz Harrosh

James and linux-scsi CCed.

Looks fine .. could someone send the patch in an applyable form (i.e. not quoted).

Sure:

Fix NULL pointer dereference during execution of Internal commands, where gdth only allocates scp, but not scp->sense_buffer. The rest of the code assumes that sense_buffer is allocated, which leads to a kernel oops e.g. on reboot (during cache flush).

Signed-off-by Sven Schnelle <[email protected]>

---

From 1795a2063eabf326c4fba49af4db6572bdd642b6 Mon Sep 17 00:00:00 2001

Quoted 94 lines · Boaz Harrosh

Quoted 93 lines · Sven Schnelle

Hi,

i'm facing the following kernel oops with the latest git:

--------------------------------------8<-------------------------------------- Stopping MD arrays...failed (no MD subsystem loaded).
Mounting root filesystem read-only...done.
Will now restart.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
PGD 7e51e067 PUD 7e6a1067 PMD 0 
Oops: 0002 [1] PREEMPT SMP 
CPU 3 
Modules linked in: nvidia(P) lm85 hwmon_vid hwmon [last unloaded: nvidia]
Pid: 0, comm: swapper Tainted: P         2.6.25-rc4-smp-00134-g84c6f60 #11 
RIP: 0010:[<ffffffff8051a97e>]  [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
RSP: 0018:ffff81007fbefed8  EFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff81007e0dda68 RCX: 0000000000000002
RDX: 00000000ffffffff RSI: 0000000000000000 RDI: ffffc20000330000
RBP: ffff81007fbeff08 R08: 0000000000000000 R09: ffff81007f01de70
R10: 0000000000000000 R11: 0000000000000050 R12: ffff810000b10480
R13: ffff810000b104ff R14: ffff81007e214200 R15: 0000000000000009
FS:  0000000000000000(0000) GS:ffff81007f802c80(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 000000007e643000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffff81007fbe6000, task ffff81007fbe4000)
Stack:  0000000000000046 ffff81007e8a93c0 0000000000000000 0000000000000000
 0000000000000018 0000000000000003 ffff81007fbeff18 ffffffff8051ab9f
 ffff81007fbeff48 ffffffff80258cc2 ffffffff8092e300 ffff81007e8a93c0
Call Trace:
 <IRQ>  [<ffffffff8051ab9f>] gdth_interrupt+0x10/0x12
 [<ffffffff80258cc2>] handle_IRQ_event+0x27/0x57
 [<ffffffff8025a055>] handle_fasteoi_irq+0x9c/0xdc
 [<ffffffff8020def0>] do_IRQ+0x88/0xfc
 [<ffffffff80209ffe>] ? default_idle+0x0/0x5f
 [<ffffffff8020b851>] ret_from_intr+0x0/0xa
 <EOI>  [<ffffffff8020a037>] ? default_idle+0x39/0x5f
 [<ffffffff8020a032>] ? default_idle+0x34/0x5f
 [<ffffffff80209ffe>] ? default_idle+0x0/0x5f
 [<ffffffff8020a11c>] ? cpu_idle+0xbf/0xf5
 [<ffffffff806cd973>] ? start_secondary+0x3e0/0x3ef


Code: 00 04 eb 15 3c 17 75 11 41 0f b6 c5 48 c1 e0 05 41 80 a4 04 92 02 00 00 fb 41 c7 86 18 01 00 00 00 00 00 00 49 8b 86 c0 00 00 00 <c6> 00 00 e9 92 01 00 00 66 89 43 24 41 8b 84 24 68 02 00 00 89 
RIP  [<ffffffff8051a97e>] __gdth_interrupt+0x96e/0xb7f
 RSP <ffff81007fbefed8>
CR2: 0000000000000000
---[ end trace e81e561a458e8791 ]---
Kernel panic - not syncing: Aiee, killing interrupt handler!

Rebooting in 5 seconds.. --------------------------------------8<--------------------------------------

From 2dc63f9f8e61fd1c89f8b4d9b2d174be1c3bfbe2 Mon Sep 17 00:00:00 2001
From: root <[email protected]>
Date: Sun, 9 Mar 2008 13:25:07 +0100
Subject:

This fixes a NULL pointer dereference during execution of Internal commands, where gdth only allocates scp, but not scp->sense_buffer. The rest of the code assumes that sense_buffer is allocated, which leads to a kernel oops e.g. on reboot (during cache flush).

So we have two choices here:

a) Allocate the sense_buffer
b) surrounding all accesses to sense_buffer with some if (!internal_command)

I'm using solution a, as this keeps code simpler.

Signed-off-by Sven Schnelle <[email protected]>

---
 drivers/scsi/gdth.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

drivers/scsi/gdth.c +6 -0

index 27ebd33..0b2080d 100644
@@ -493,6 +493,12 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     if (!scp)
         return -ENOMEM;
 
+    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
+    if (!scp->sense_buffer) {
+	kfree(scp);
+	return -ENOMEM;
+    }
+
     scp->device = sdev;
     memset(&cmndinfo, 0, sizeof(cmndinfo));
 

Quoted 9 lines

drivers/scsi/gdth.c int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd, · line 525

Quoted 8 lines

drivers/scsi/gdth.c +1 -0

@@ -513,6 +519,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     rval = cmndinfo.status;
     if (info)
         *info = cmndinfo.info;
+    kfree(scp->sense_buffer);
     kfree(scp);
     return rval;
 }

James and linux-scsi CCed.

Looks fine .. could someone send the patch in an applyable form (i.e. not quoted).

Quoted 4 lines

James it looks reasonable. It's a fallout from the sense_buffer separation patches.

Reviewed-by Boaz Harrosh <[email protected]>

I will submit solution b) above as part of my sense handling patchset.

Um, that looks like it will be a bit nasty, so the simpler fix is probably the better one (even if the sense buffer simply gets ignored).

The best long term fix for GDTH is probably to make it behave more like a standard raid driver, so it includes a translation layer from scsi commands to gdth commands and then the __gdth_execute() can be reformulated as gdth command injection and we don't have to muck about with upper layer objects like SCSI commands.

James

I would like to fix this better by calling scsi_get/put_command but there is something fundamental that bothers me with isd200 driver. I can see that an isd200_info struct is allocated and put on a struct us_data->extra. But as I understand the code, the struct us_data is associated with a scsi_host not a scsi_device. Are we guarantied that we have only one scsi_device per host at all times?

I would like to fix this better by calling scsi_get/put_command but there is something fundamental that bothers me with isd200 driver. I can see that an isd200_info struct is allocated and put on a struct us_data->extra. But as I understand the code, the struct us_data is associated with a scsi_host not a scsi_device. Are we guarantied that we have only one scsi_device per host at all times?

If not than the resources in isd200_info that are related to a request_queue and are used from a .queuecommand code-path are not thread safe. (Like the srb member)

If Yes, then where in the code initialization sequence is the earliest place I can get to a scsi_device. I could do that on first call to .queuecommand but I would rather do it in a place that I could use GFP_KERNEL for allocation of the extra command? (Same question on the tear down of the scsi_device)

(And one more stupid question. Why does isd200_init_info allocates the info structure but the isd200_free_info_ptrs does not free it, it kind of limits the way it can be allocated, no?)

Quoted 11 lines · Boaz Harrosh

I would like to fix this better by calling scsi_get/put_command but there is something fundamental that bothers me with isd200 driver. I can see that an isd200_info struct is allocated and put on a struct us_data->extra. But as I understand the code, the struct us_data is associated with a scsi_host not a scsi_device. Are we guarantied that we have only one scsi_device per host at all times?

If not than the resources in isd200_info that are related to a request_queue and are used from a .queuecommand code-path are not thread safe. (Like the srb member)

Quoted 4 lines

If Yes, then where in the code initialization sequence is the earliest place I can get to a scsi_device. I could do that on first call to .queuecommand but I would rather do it in a place that I could use GFP_KERNEL for allocation of the extra command? (Same question on the tear down of the scsi_device)

You can first get to the scsi_device in isd200_ata_command().

The last
place you can get to the scsi_device (if one exists!) is quiesce_and_remove_host() -- but that's part of the core, not specific to isd200.

Quoted 3 lines

(And one more stupid question. Why does isd200_init_info allocates the info structure but the isd200_free_info_ptrs does not free it, it kind of limits the way it can be allocated, no?)

Not at all. isd200_free_info_ptrs() frees everything pointed to by the info structure, and the info structure itself is freed later on by the usb-storage core in usb_stor_release_resources().

If you wanted to free it in isd200_free_info_ptrs() you could; just make sure to set us->extra to NULL when you do, to avoid a double-free error.

Alan Stern

Quoted 1 line

You can first get to the scsi_device in isd200_ata_command().

I was afraid of that. I don't think I want to call scsi_get_command from within .queuecommand. I will leave the code hacked as today.

Quoted 5 lines

The last
place you can get to the scsi_device (if one exists!) is quiesce_and_remove_host() -- but that's part of the core, not specific to isd200.

Here two, it looks like I need to introduce a new function pointer for isd200

I'll leave it for now. Though I know this is not the last I'll see of this driver.

Quoted 8 lines

Quoted 3 lines

(And one more stupid question. Why does isd200_init_info allocates the info structure but the isd200_free_info_ptrs does not free it, it kind of limits the way it can be allocated, no?)

Not at all. isd200_free_info_ptrs() frees everything pointed to by the info structure, and the info structure itself is freed later on by the usb-storage core in usb_stor_release_resources().

OK so in isd200_get_inquiry_data() at the end near the call to:

			us->extra_destructor(info);
			us->extra = NULL;

It leaks the info.

Please ACK the first patch sent, so James can put it in scsi-rc-fixes as part of the sense_buffer effort for 2.6.25-rc

Quoted 3 lines · Boaz Harrosh

Quoted 1 line

You can first get to the scsi_device in isd200_ata_command().

I was afraid of that. I don't think I want to call scsi_get_command from within .queuecommand. I will leave the code hacked as today.

What are you talking about? isd200_ata_command isn't called by queuecommand.

Quoted 6 lines

Quoted 5 lines

The last
place you can get to the scsi_device (if one exists!) is quiesce_and_remove_host() -- but that's part of the core, not specific to isd200.

Here two, it looks like I need to introduce a new function pointer for isd200

Why? And why do you need to get to the scsi_device in the first place?

Quoted 2 lines

I'll leave it for now. Though I know this is not the last I'll see of this driver.

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

(And one more stupid question. Why does isd200_init_info allocates the info structure but the isd200_free_info_ptrs does not free it, it kind of limits the way it can be allocated, no?)

Not at all. isd200_free_info_ptrs() frees everything pointed to by the info structure, and the info structure itself is freed later on by the usb-storage core in usb_stor_release_resources().

OK so in isd200_get_inquiry_data() at the end near the call to:

			us->extra_destructor(info);
			us->extra = NULL;

Quoted 1 line

It leaks the info.

Yes. The three lines of code there are unnecessary. You should remove them (and the comment) instead of adding more somewhere else. Or if you want to keep them, just add a line to kfree(us->extra) before us->extra is set to NULL.

Quoted 3 lines

Please ACK the first patch sent, so James can put it in scsi-rc-fixes as part of the sense_buffer effort for 2.6.25-rc

The first patch is okay except for style violations:

Review struct isd200_info { · line 297

Quoted 5 lines

(quoted diff) +1 -0

@@ -294,6 +294,7 @@ struct isd200_info {
 	unsigned char MaxLUNs;
 	struct scsi_cmnd srb;
 	struct scatterlist sg;
+	u8*	sense_buffer;

This should be

	u8 *sense_buffer;

And

Review

Quoted 6 lines

(quoted diff) +0 -0

 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;

(quoted diff) +2 -0

 		}
+		else
+			info->srb.sense_buffer = info->sense_buffer;

The "else" should go on the same line as the closing brace, and it should have its own opening brace.

Alan Stern

--

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Quoted 3 lines · Boaz Harrosh

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Did you run this through checkpatch?

Acked-by Alan Stern <[email protected]>

Alan Stern

--

Quoted 6 lines · Alan Stern

Quoted 3 lines · Boaz Harrosh

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Quoted 7 lines

Did you run this through checkpatch?

Acked-by Alan Stern <[email protected]>

Alan Stern

--

No I did not, Thanks. Here it is again.
Again this is for 2.6.25 rc-fixes a NULL dereference bugfix!

---
From: Boaz Harrosh <[email protected]>
Date: Tue, 11 Mar 2008 17:23:06 +0200
Subject: [PATCH] isd200: Allocate sense_buffer for hacked up scsi_cmnd

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/usb/storage/isd200.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/usb/storage/isd200.c +0 -0

index 2ae1e86..ac1764b 100644

drivers/usb/storage/isd200.c +1 -0

 };
 
 

@@ -1469,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->sense_buffer);
 	}
 }
 

Quoted 30 lines · Boaz Harrosh

Quoted 5 lines · Alan Stern

Quoted 3 lines · Boaz Harrosh

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Quoted 7 lines

Did you run this through checkpatch?

Acked-by Alan Stern <[email protected]>

Alan Stern

--

No I did not, Thanks. Here it is again.
Again this is for 2.6.25 rc-fixes a NULL dereference bugfix!

---
From: Boaz Harrosh <[email protected]>
Date: Tue, 11 Mar 2008 17:23:06 +0200
Subject: [PATCH] isd200: Allocate sense_buffer for hacked up scsi_cmnd

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/usb/storage/isd200.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/usb/storage/isd200.c struct isd200_info { · line 297

Quoted 9 lines

drivers/usb/storage/isd200.c +1 -0

index 2ae1e86..ac1764b 100644
@@ -294,6 +294,7 @@ struct isd200_info {
 	unsigned char MaxLUNs;
 	struct scsi_cmnd srb;
 	struct scatterlist sg;
+	u8 *sense_buffer;

There's no real need to add this parameter, since all you're doing is assigning it to srb.sense_buffer, there's no need to have an extra pointer for it.

Quoted 12 lines

drivers/usb/storage/isd200.c +1 -0

 };
 
 

@@ -1469,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->sense_buffer);
 	}
 }
 

drivers/usb/storage/isd200.c static int isd200_init_info(struct us_data *us) · line 1499

Quoted 5 lines

drivers/usb/storage/isd200.c +1 -1

@@ -1494,11 +1496,13 @@ static int isd200_init_info(struct us_data *us)
 				kzalloc(sizeof(struct hd_driveid), GFP_KERNEL);
 		info->RegsBuf = (unsigned char *)
 				kmalloc(sizeof(info->ATARegs), GFP_KERNEL);
-		if (!info->id || !info->RegsBuf) {
+		info->sense_buffer = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);

^ kzalloc is probably best

drivers/usb/storage/isd200.c static int isd200_init_info(struct us_data *us) · line 1502

Quoted 3 lines

drivers/usb/storage/isd200.c +1 -0

+		if (!info->id || !info->RegsBuf || !info->sense_buffer) {
 			isd200_free_info_ptrs(info);
 			kfree(info);

Needs to be a kfree(info->sense_buffer) to avoid leaks if the others couldn't allocate ... it also looks like there are missing kfree(info->id) and (info->RegsBuf) that fix leaks that aren't part of this patch.

Quoted 7 lines

drivers/usb/storage/isd200.c +2 -1

 			retStatus = ISD200_ERROR;
-		}
+		} else
+			info->srb.sense_buffer = info->sense_buffer;
 	}
 

drivers/usb/storage/isd200.c static int isd200_init_info(struct us_data *us) · line 1508

Quoted 1 line

drivers/usb/storage/isd200.c +0 -0

 	if (retStatus == ISD200_GOOD) {

James

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/usb/storage/isd200.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

drivers/usb/storage/isd200.c +1 -0

index 4f2d143..971d13d 100644
@@ -1470,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->srb.sense_buffer);
 	}
 }
 

Quoted 22 lines · On Wed, 12 Mar 2008 19:20:09 +0200 Boaz Harrosh <[email protected]> wrote:

Since the separation of sense_buffer from scsi_cmnd, Drivers that hack their own struct scsi_cmnd like here isd200, must also take care of their own sense_buffer.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/usb/storage/isd200.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

drivers/usb/storage/isd200.c +1 -0

index 4f2d143..971d13d 100644
@@ -1470,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->srb.sense_buffer);
 	}
 }
 

drivers/usb/storage/isd200.c static int isd200_init_info(struct us_data *us) · line 1504

Quoted 11 lines

drivers/usb/storage/isd200.c +3 -1

@@ -1495,7 +1496,9 @@ static int isd200_init_info(struct us_data *us)
 				kzalloc(sizeof(struct hd_driveid), GFP_KERNEL);
 		info->RegsBuf = (unsigned char *)
 				kmalloc(sizeof(info->ATARegs), GFP_KERNEL);
-		if (!info->id || !info->RegsBuf) {
+		info->srb.sense_buffer =
+				kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
+		if (!info->id || !info->RegsBuf || !info->srb.sense_buffer) {
 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;

I've thoroughly lost the plot here.

Is this needed in 2.6.25? If so, why?

Quoted 8 lines · Alan Stern

Quoted 3 lines · Boaz Harrosh

Quoted 1 line

You can first get to the scsi_device in isd200_ata_command().

I was afraid of that. I don't think I want to call scsi_get_command from within .queuecommand. I will leave the code hacked as today.

What are you talking about? isd200_ata_command isn't called by queuecommand.

Quoted 10 lines

Quoted 6 lines

Quoted 5 lines

The last
place you can get to the scsi_device (if one exists!) is quiesce_and_remove_host() -- but that's part of the core, not specific to isd200.

Here two, it looks like I need to introduce a new function pointer for isd200

Why? And why do you need to get to the scsi_device in the first place?

Quoted 2 lines

I'll leave it for now. Though I know this is not the last I'll see of this driver.

OK Now I see isd200_ata_command() is called from a usb.c internal thread.

What I need to do is call scsi_get_command(scsi_device*) on first invocation. Now for the call to scsi_put_command()? At the time of the call to isd200_free_info_ptrs() do you think I still have a valid scsi_device at this point?

What I will do is this. I will resend my original patch with your comments fixed. This is for the 2.6.25-rc. And I will send another patch that uses the proper scsi_get/put_command() for testing and inclusion into the 2.6.26 kernel. Please ACK on the patch

Quoted 11 lines

Quoted 11 lines

Quoted 7 lines

Quoted 3 lines

(And one more stupid question. Why does isd200_init_info allocates the info structure but the isd200_free_info_ptrs does not free it, it kind of limits the way it can be allocated, no?)

Not at all. isd200_free_info_ptrs() frees everything pointed to by the info structure, and the info structure itself is freed later on by the usb-storage core in usb_stor_release_resources().

OK so in isd200_get_inquiry_data() at the end near the call to:

			us->extra_destructor(info);
			us->extra = NULL;

Quoted 5 lines

Quoted 1 line

It leaks the info.

Yes. The three lines of code there are unnecessary. You should remove them (and the comment) instead of adding more somewhere else. Or if you want to keep them, just add a line to kfree(us->extra) before us->extra is set to NULL.

How are they unnecessary? who will free them? other wise they will only be freed at the very end. And that is only because usb_stor_transparent_scsi_command() does not need any us->extra of it's own. But if ever it will, then this code buried here will become a leak.

And I disagree. with your solution. The module that did the allocation should do the freeing. The above is just an example of what happens with bad programing style. the core should not have attempted a free on a void pointer just so protocols can get lazy and not register a destructor. Other wise we do not learn from passed mistakes and keep doing the same errors. The free should always be at same file right next to the alloc. (And don't get me started on the flexibility that enables)

I keep the patch as it is, I recommend to commit it for solving the leak.

rrr only I cannot do that because the destructor does not have access to the us_data that contains it. As I said, very ugly. New patch attached.

Boaz
---
From: Boaz Harrosh <[email protected]>
Date: Tue, 11 Mar 2008 20:30:53 +0200
Subject: [PATCH] isd200: Fix memory leak in isd200_get_inquiry_data

if the inquiry fails then the info structure on
us->extra was not freed.

Signed-off-by Boaz Harrosh <[email protected]>

OK Now I see isd200_ata_command() is called from a usb.c internal thread.

What I need to do is call scsi_get_command(scsi_device*) on first invocation.

Now for the call to scsi_put_command()? At the time of the call to isd200_free_info_ptrs() do you think I still have a valid scsi_device at this point?

What I will do is this. I will resend my original patch with your comments fixed. This is for the 2.6.25-rc. And I will send another patch that uses the proper scsi_get/put_command() for testing and inclusion into the 2.6.26 kernel. Please ACK on the patch

Quoted 4 lines

Yes. The three lines of code there are unnecessary. You should remove them (and the comment) instead of adding more somewhere else. Or if you want to keep them, just add a line to kfree(us->extra) before us->extra is set to NULL.

How are they unnecessary? who will free them? other wise they will only be freed at the very end.

And that is only because usb_stor_transparent_scsi_command() does not need any us->extra of it's own. But if ever it will, then this code buried here will become a leak.