[PATCH] drivers/scsi: Check NULL for kmalloc() return

[PATCH] drivers/scsi: Check NULL for kmalloc() return

[Davidlohr Bueso A.]

Verify that ch->dt is not NULL before using it:
ch-dt[elem] = value;

Signed-off-by: Davidlohr Bueso

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

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 7b1633a..96cbd20 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -353,6 +353,10 @@ ch_readconfig(scsi_changer *ch)
        /* look up the devices of the data transfer elements */
        ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
                         GFP_KERNEL);
+
+       if (!ch->dt)
+               return -ENOMEM;
+
        for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
                id  = -1;
                lun = 0;

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[Julia Lawall]

On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:

> Verify that ch->dt is not NULL before using it:
> ch-dt[elem] = value;

It looks like buffer should be freed as well?

julia

> Signed-off-by: Davidlohr Bueso
> 
> ---
>  drivers/scsi/ch.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index 7b1633a..96cbd20 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -353,6 +353,10 @@ ch_readconfig(scsi_changer *ch)
>         /* look up the devices of the data transfer elements */
>         ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
>                          GFP_KERNEL);
> +
> +       if (!ch->dt)
> +               return -ENOMEM;
> +
>         for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
>                 id  = -1;
>                 lun = 0;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[Davidlohr Bueso A.]

On Fri, Aug 07, 2009 at 08:54:44PM +0200, Julia Lawall wrote:
> On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
> 
> > Verify that ch->dt is not NULL before using it:
> > ch-dt[elem] = value;
> 
> It looks like buffer should be freed as well?

The way I see it, this is done in ch_remove()

Davidlohr

> 
> julia
> 
> > Signed-off-by: Davidlohr Bueso
> > 
> > ---
> >  drivers/scsi/ch.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> > index 7b1633a..96cbd20 100644
> > --- a/drivers/scsi/ch.c
> > +++ b/drivers/scsi/ch.c
> > @@ -353,6 +353,10 @@ ch_readconfig(scsi_changer *ch)
> >         /* look up the devices of the data transfer elements */
> >         ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
> >                          GFP_KERNEL);
> > +
> > +       if (!ch->dt)
> > +               return -ENOMEM;
> > +
> >         for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
> >                 id  = -1;
> >                 lun = 0;
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[James Bottomley]

On Fri, 2009-08-07 at 14:39 -0400, Davidlohr Bueso A. wrote:
> Verify that ch->dt is not NULL before using it:
> ch-dt[elem] = value;
> 
> Signed-off-by: Davidlohr Bueso

This actually needs to be an email address, not just a name, so

Signed-off-by: Davidlohr Bueso <dave@gnu.org>

> ---
>  drivers/scsi/ch.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index 7b1633a..96cbd20 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -353,6 +353,10 @@ ch_readconfig(scsi_changer *ch)
>         /* look up the devices of the data transfer elements */
>         ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
>                          GFP_KERNEL);
> +
> +       if (!ch->dt)
> +               return -ENOMEM;

This isn't quite right because you'll leak the memory allocated for
buffer here.

For bonus points, the GFP_DMA in the kzalloc() of buffer is bogus ...
this routine can sleep at all points.

James

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[Julia Lawall]

On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:

> On Fri, Aug 07, 2009 at 08:54:44PM +0200, Julia Lawall wrote:
> > On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
> > 
> > > Verify that ch->dt is not NULL before using it:
> > > ch-dt[elem] = value;
> > 
> > It looks like buffer should be freed as well?
> 
> The way I see it, this is done in ch_remove()

I don't see that at all.  buffer appears to be a variable that is local to 
ch_readconfig and is passed down to other functions, but never saved 
anywhere.  Furthermore buffer is freed in the normal exit of the function, 
so it seems likely that it should be freed on an early exit as well.

julia

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[Davidlohr Bueso A.]

On Fri, Aug 07, 2009 at 09:56:48PM +0200, Julia Lawall wrote:
> On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
> 
> > On Fri, Aug 07, 2009 at 08:54:44PM +0200, Julia Lawall wrote:
> > > On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
> > > 
> > > > Verify that ch->dt is not NULL before using it:
> > > > ch-dt[elem] = value;
> > > 
> > > It looks like buffer should be freed as well?
> > 
> > The way I see it, this is done in ch_remove()
> 
> I don't see that at all.  buffer appears to be a variable that is local to 
> ch_readconfig and is passed down to other functions, but never saved 
> anywhere.  Furthermore buffer is freed in the normal exit of the function, 
> so it seems likely that it should be freed on an early exit as well.

Sorry, misread, for some reason I thought you were talking about freeing ch->dt, correting patch.

Thanks,
Davidlohr

Signed-off-by: Davidlohr Bueso <dave@gnu.org>

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

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 7b1633a..bb42ceb 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -353,6 +353,12 @@ ch_readconfig(scsi_changer *ch)
        /* look up the devices of the data transfer elements */
        ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
                         GFP_KERNEL);
+
+       if (!ch->dt) {
+               free(buffer);
+               return -ENOMEM;
+       }
+
        for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
                id  = -1;
                lun = 0;

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[Hannes Eder]

On Fri, Aug 7, 2009 at 22:17, Davidlohr Bueso A.<dave@gnu.org> wrote:
> On Fri, Aug 07, 2009 at 09:56:48PM +0200, Julia Lawall wrote:
>> On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
>>
>> > On Fri, Aug 07, 2009 at 08:54:44PM +0200, Julia Lawall wrote:
>> > > On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
>> > >
>> > > > Verify that ch->dt is not NULL before using it:
>> > > > ch-dt[elem] = value;
>> > >
>> > > It looks like buffer should be freed as well?
>> >
>> > The way I see it, this is done in ch_remove()
>>
>> I don't see that at all.  buffer appears to be a variable that is local to
>> ch_readconfig and is passed down to other functions, but never saved
>> anywhere.  Furthermore buffer is freed in the normal exit of the function,
>> so it seems likely that it should be freed on an early exit as well.
>
> Sorry, misread, for some reason I thought you were talking about freeing ch->dt, correting patch.
>
> Thanks,
> Davidlohr
>
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
>
> ---
>  drivers/scsi/ch.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index 7b1633a..bb42ceb 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -353,6 +353,12 @@ ch_readconfig(scsi_changer *ch)
>        /* look up the devices of the data transfer elements */
>        ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
>                         GFP_KERNEL);
> +
> +       if (!ch->dt) {
> +               free(buffer);

kfree(buffer) ?

> +               return -ENOMEM;
> +       }
> +
>        for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
>                id  = -1;
>                lun = 0;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return

[Davidlohr Bueso A.]

On Fri, Aug 07, 2009 at 10:33:36PM +0200, Hannes Eder wrote:
> On Fri, Aug 7, 2009 at 22:17, Davidlohr Bueso A.<dave@gnu.org> wrote:
> > On Fri, Aug 07, 2009 at 09:56:48PM +0200, Julia Lawall wrote:
> >> On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
> >>
> >> > On Fri, Aug 07, 2009 at 08:54:44PM +0200, Julia Lawall wrote:
> >> > > On Fri, 7 Aug 2009, Davidlohr Bueso A. wrote:
> >> > >
> >> > > > Verify that ch->dt is not NULL before using it:
> >> > > > ch-dt[elem] = value;
> >> > >
> >> > > It looks like buffer should be freed as well?
> >> >
> >> > The way I see it, this is done in ch_remove()
> >>
> >> I don't see that at all.  buffer appears to be a variable that is local to
> >> ch_readconfig and is passed down to other functions, but never saved
> >> anywhere.  Furthermore buffer is freed in the normal exit of the function,
> >> so it seems likely that it should be freed on an early exit as well.
> >
> > Sorry, misread, for some reason I thought you were talking about freeing ch->dt, correting patch.
> >
> > Thanks,
> > Davidlohr
> >
> > Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> >
> > ---
> >  drivers/scsi/ch.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> > index 7b1633a..bb42ceb 100644
> > --- a/drivers/scsi/ch.c
> > +++ b/drivers/scsi/ch.c
> > @@ -353,6 +353,12 @@ ch_readconfig(scsi_changer *ch)
> >        /* look up the devices of the data transfer elements */
> >        ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
> >                         GFP_KERNEL);
> > +
> > +       if (!ch->dt) {
> > +               free(buffer);
> 
> kfree(buffer) ?

Wow, unbelievable, that'll teach me!

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 drivers/scsi/ch.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 7b1633a..fe11c1d 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -353,6 +353,12 @@ ch_readconfig(scsi_changer *ch)
        /* look up the devices of the data transfer elements */
        ch->dt = kmalloc(ch->counts[CHET_DT]*sizeof(struct scsi_device),
                         GFP_KERNEL);
+
+       if (!ch->dt) {
+               kfree(buffer);
+               return -ENOMEM;
+       }
+
        for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
                id  = -1;
                lun = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html