Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Boaz Harrosh]

On Sun, Mar 09 2008 at 14:41 +0200, Sven Schnelle <svens@stackframe.org> wrote:
> 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 <root@apollo.bitebene.org>
> 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 <svens@stackframe.org>
> ---
>  drivers/scsi/gdth.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 27ebd33..0b2080d 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -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));
>  
> @@ -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 <bharrosh@panasas.com>

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

Boaz

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[James Bottomley]

On Mon, 2008-03-10 at 17:20 +0200, Boaz Harrosh wrote:
> On Sun, Mar 09 2008 at 14:41 +0200, Sven Schnelle <svens@stackframe.org> wrote:
> > 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 <root@apollo.bitebene.org>
> > 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 <svens@stackframe.org>
> > ---
> >  drivers/scsi/gdth.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> > index 27ebd33..0b2080d 100644
> > --- a/drivers/scsi/gdth.c
> > +++ b/drivers/scsi/gdth.c
> > @@ -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));
> >  
> > @@ -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).

> James it looks reasonable. It's a fallout from the sense_buffer separation patches.
> Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>
> 
> 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

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Sven Schnelle]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Mon, 2008-03-10 at 17:20 +0200, Boaz Harrosh wrote:
>> 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 <svens@stackframe.org>
---
 drivers/scsi/gdth.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 27ebd33..0b2080d 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -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));
 
@@ -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;
 }
-- 
1.5.4.3

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Boaz Harrosh]

On Mon, Mar 10 2008 at 23:50 +0200, Sven Schnelle <svens@bitebene.org> wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
>> On Mon, 2008-03-10 at 17:20 +0200, Boaz Harrosh wrote:
>>> 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 <svens@stackframe.org>
> ---
<snip>

Hi Sven.

Do you have gdth HW for testing patches?
I'm anticipating more scsi-ml changes in that regard in near future and would like
a more permanent solution for gdth. Could you please try below patch in place of
your patch and see if it works.

Thanks in advance

James Hi
do you think we should keep Sven's patch for the rc-fixes and my solution
for the next kernel? For the reason that my patch might be theoretically dangerous 
in regard to locking, queue-life-time, and such side effects?

Boaz
--- 
>From 1795a2063eabf326c4fba49af4db6572bdd642b6 Mon Sep 17 00:00:00 2001
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Tue, 11 Mar 2008 17:42:06 +0200
Subject: [PATCH] gdth: Use scsi_get_command for gdth internal commands

As found by: Sven Schnelle <svens@bitebene.org>
NULL pointer dereference bug 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).

Fix this by leting scsi-ml allocate the command for us, in anticipation
of future changes to commands allocation.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/gdth.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 4560e39..643e7d6 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -448,7 +448,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;
 
-    scp = kzalloc(sizeof(*scp), GFP_KERNEL);
+    scp = scsi_get_command(sdev, GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
 
@@ -472,7 +472,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     rval = cmndinfo.status;
     if (info)
         *info = cmndinfo.info;
-    kfree(scp);
+    scsi_put_command(scp);
     return rval;
 }
 
-- 
1.5.3.3

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Boaz Harrosh]

On Mon, Mar 10 2008 at 23:12 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Mon, 2008-03-10 at 17:20 +0200, Boaz Harrosh wrote:
>> On Sun, Mar 09 2008 at 14:41 +0200, Sven Schnelle <svens@stackframe.org> wrote:
>>> 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 <root@apollo.bitebene.org>
>>> 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 <svens@stackframe.org>
>>> ---
>>>  drivers/scsi/gdth.c |    7 +++++++
>>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
>>> index 27ebd33..0b2080d 100644
>>> --- a/drivers/scsi/gdth.c
>>> +++ b/drivers/scsi/gdth.c
>>> @@ -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));
>>>  
>>> @@ -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).
> 
>> James it looks reasonable. It's a fallout from the sense_buffer separation patches.
>> Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>
>>
>> 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
> 
> 

James Hi
There is one more such bug in USB's isd200 driver. Below is a patch for 
rc-fixes.

Alen && Matthew Dharm hi. (If I missed someone please send it his way)

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?)

Please advise.

Here is the patch
---
From: Boaz Harrosh <bharrosh@panasas.com>
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 <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 2ae1e86..9eb2cdf 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -294,6 +294,7 @@ struct isd200_info {
 	unsigned char MaxLUNs;
 	struct scsi_cmnd srb;
 	struct scatterlist sg;
+	u8*	sense_buffer;
 };
 
 
@@ -1469,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->sense_buffer);
 	}
 }
 
@@ -1494,11 +1496,14 @@ 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);
+		if (!info->id || !info->RegsBuf || !info->sense_buffer) {
 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;
 		}
+		else
+			info->srb.sense_buffer = info->sense_buffer;
 	}
 
 	if (retStatus == ISD200_GOOD) {
-- 
1.5.3.3

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Matthew Dharm]

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

On Tue, Mar 11, 2008 at 06:16:16PM +0200, Boaz Harrosh wrote:
> 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?

Yes.  We allocate a scsi_host for each USB device that we see.  The only
time you can have more than one scsi_device is in the case of multi-target
devices, of which the ISD-200 is NOT one.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Hey, has anyone seen the Microsoft sales guy? It's his feeding time...
					-- Mike
User Friendly, 4/17/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Alan Stern]

On Tue, 11 Mar 2008, Boaz Harrosh wrote:

> 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)

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.

> (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

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Boaz Harrosh]

On Tue, Mar 11 2008 at 20:07 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 11 Mar 2008, Boaz Harrosh wrote:
> 
>> 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)
> 
> 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.

> 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.

>> (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.

> 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.
> 

Patch attached to fix the above fix

> Alan Stern
> 

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 below patch you can put threw the USB tree, it's for you.
---
From: Boaz Harrosh <bharrosh@panasas.com>
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 call to us->extra_destructor()
is assumed to also free the info structure. So make that
so in isd200_free_info_ptrs()

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 9eb2cdf..77f4754 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1471,6 +1471,9 @@ static void isd200_free_info_ptrs(void *info_)
 		kfree(info->id);
 		kfree(info->RegsBuf);
 		kfree(info->sense_buffer);
+		kfree(info);
+		us->extra = NULL;
+		us->extra_destructor = NULL;
 	}
 }
 
-- 
1.5.3.3

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Alan Stern]

On Tue, 11 Mar 2008, Boaz Harrosh wrote:

> > 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.

> > 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?

> I'll leave it for now. Though I know this is not the last I'll see of this driver.
> 
> >> (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.

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.

> 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:

> @@ -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

>  			isd200_free_info_ptrs(info);
>  			kfree(info);
>  			retStatus = ISD200_ERROR;
>  		}
> +		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

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Boaz Harrosh]

On Tue, Mar 11 2008 at 21:18 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 11 Mar 2008, Boaz Harrosh wrote:
> 
>>> 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.
> 
>>> 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?
> 
>> 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

>>>> (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.
> 
> 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.

> 
>> 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:
> 
>> @@ -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
> 
>>  			isd200_free_info_ptrs(info);
>>  			kfree(info);
>>  			retStatus = ISD200_ERROR;
>>  		}
>> +		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
> 
> --

The important sense_buffer bugfix is posted as reply to this mail

Boaz

[PATCH] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[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.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 2ae1e86..ac1764b 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -294,6 +294,7 @@ struct isd200_info {
 	unsigned char MaxLUNs;
 	struct scsi_cmnd srb;
 	struct scatterlist sg;
+	u8* sense_buffer;
 };
 
 
@@ -1469,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->sense_buffer);
 	}
 }
 
@@ -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);
+		if (!info->id || !info->RegsBuf || !info->sense_buffer) {
 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;
-		}
+		} else
+			info->srb.sense_buffer = info->sense_buffer;
 	}
 
 	if (retStatus == ISD200_GOOD) {
-- 
1.5.3.3

Re: [PATCH] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[Alan Stern]

On Wed, 12 Mar 2008, Boaz Harrosh 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.

Did you run this through checkpatch?

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

[PATCH resend] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[Boaz Harrosh]

On Wed, Mar 12 2008 at 17:10 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 12 Mar 2008, Boaz Harrosh 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.
> 
> Did you run this through checkpatch?
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> 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 <bharrosh@panasas.com>
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 <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 2ae1e86..ac1764b 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -294,6 +294,7 @@ struct isd200_info {
 	unsigned char MaxLUNs;
 	struct scsi_cmnd srb;
 	struct scatterlist sg;
+	u8 *sense_buffer;
 };
 
 
@@ -1469,6 +1470,7 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
+		kfree(info->sense_buffer);
 	}
 }
 
@@ -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);
+		if (!info->id || !info->RegsBuf || !info->sense_buffer) {
 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;
-		}
+		} else
+			info->srb.sense_buffer = info->sense_buffer;
 	}
 
 	if (retStatus == ISD200_GOOD) {
-- 
1.5.3.3

Re: [PATCH resend] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[James Bottomley]

On Wed, 2008-03-12 at 17:24 +0200, Boaz Harrosh wrote:
> On Wed, Mar 12 2008 at 17:10 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 12 Mar 2008, Boaz Harrosh 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.
> > 
> > Did you run this through checkpatch?
> > 
> > Acked-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > 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 <bharrosh@panasas.com>
> 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 <bharrosh@panasas.com>
> ---
>  drivers/usb/storage/isd200.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 2ae1e86..ac1764b 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -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.

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

>  			retStatus = ISD200_ERROR;
> -		}
> +		} else
> +			info->srb.sense_buffer = info->sense_buffer;
>  	}
>  
>  	if (retStatus == ISD200_GOOD) {

James

Re: [PATCH resend] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[Boaz Harrosh]

On Wed, Mar 12 2008 at 18:54 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Wed, 2008-03-12 at 17:24 +0200, Boaz Harrosh wrote:
>> On Wed, Mar 12 2008 at 17:10 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
>>> On Wed, 12 Mar 2008, Boaz Harrosh 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.
>>> Did you run this through checkpatch?
>>>
>>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>>>
>>> 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 <bharrosh@panasas.com>
>> 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 <bharrosh@panasas.com>
>> ---
>>  drivers/usb/storage/isd200.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
>> index 2ae1e86..ac1764b 100644
>> --- a/drivers/usb/storage/isd200.c
>> +++ b/drivers/usb/storage/isd200.c
>> @@ -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.
> 

You are right, thanks.

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

No! that's fine. There is no leaks. the call to isd200_free_info_ptrs takes
care of that.

> 
>>  			retStatus = ISD200_ERROR;
>> -		}
>> +		} else
>> +			info->srb.sense_buffer = info->sense_buffer;
>>  	}
>>  
>>  	if (retStatus == ISD200_GOOD) {
> 
> James
> 
> 
Resend as reply to this mail.

Boaz

[PATCH ver3] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[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.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 4f2d143..971d13d 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -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);
 	}
 }
 
@@ -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;
-- 
1.5.3.3

Re: [PATCH ver3] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[Andrew Morton]

On Wed, 12 Mar 2008 19:20:09 +0200
Boaz Harrosh <bharrosh@panasas.com> 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 <bharrosh@panasas.com>
> ---
>  drivers/usb/storage/isd200.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 4f2d143..971d13d 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -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);
>  	}
>  }
>  
> @@ -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?

Thanks.

Re: [PATCH ver3] isd200: Allocate sense_buffer for hacked up scsi_cmnd

[James Bottomley]

On Thu, 2008-03-13 at 13:01 -0700, Andrew Morton wrote:
> On Wed, 12 Mar 2008 19:20:09 +0200
> Boaz Harrosh <bharrosh@panasas.com> 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 <bharrosh@panasas.com>
> > ---
> >  drivers/usb/storage/isd200.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> > index 4f2d143..971d13d 100644
> > --- a/drivers/usb/storage/isd200.c
> > +++ b/drivers/usb/storage/isd200.c
> > @@ -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);
> >  	}
> >  }
> >  
> > @@ -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.

Don't worry ... that's why life gave us SCSI maintainers ...

>   Is this needed in 2.6.25?  If so, why?

Yes.  Because the changes that separate the sense buffer from the
commmand allocation which cause this bug went in in the merge window for
2.6.25

James

[PATCH] isd200: Fix memory leak in isd200_get_inquiry_data

[Boaz Harrosh]

On Wed, Mar 12 2008 at 15:07 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On Tue, Mar 11 2008 at 21:18 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Tue, 11 Mar 2008, Boaz Harrosh wrote:
>>
>>>> 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.
>>
>>>> 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?
>>
>>> 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
> 
>>>>> (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.
>> 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 <bharrosh@panasas.com>
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 <bharrosh@panasas.com>
---
 drivers/usb/storage/isd200.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index ac1764b..2de1f1e 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1231,6 +1231,7 @@ static int isd200_get_inquiry_data( struct us_data *us )
 	    
 			/* Free driver structure */	    
 			us->extra_destructor(info);
+			kfree(info);
 			us->extra = NULL;
 			us->extra_destructor = NULL;
 		}
-- 
1.5.3.3

Re: [PATCH] isd200: Fix memory leak in isd200_get_inquiry_data

[Alan Stern]

On Wed, 12 Mar 2008, Boaz Harrosh wrote:

> 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 <bharrosh@panasas.com>
> 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 <bharrosh@panasas.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

[Alan Stern]

On Wed, 12 Mar 2008, Boaz Harrosh wrote:

> 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.

Why?

> 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?

Definitely not.

> 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

Okay.

> > 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.

That's what I meant.

> 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.

Any additional resources needed by the transparent-SCSI handler will be 
added directly into the main us_data structure; they won't be part of 
us->extra.  That hook was meant specifically for use by the nonstandard 
subdrivers.

But on the whole you are right, and I agree with the change in your 
follow-up patch.

Alan Stern