Skip to contentSkip to conversation

drivers/scsi: Check NULL for kmalloc() return

PATCH In review Davidlohr Bueso A. 4 participants 8 messages Aug 7, 2009

Subsystems (2)
SCSI Kernel janitors
lore
Share & tools
lore

Davidlohr Bueso A.

author
#0 lore raw

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

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(-)
drivers/scsi/ch.c +4 -0
index 7b1633a..96cbd20 100644
@@ -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;
#1 lore raw
Quoted 2 lines · Davidlohr Bueso A.

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

It looks like buffer should be freed as well?

julia

Quoted 26 lines
Signed-off-by Davidlohr Bueso
---
 drivers/scsi/ch.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/scsi/ch.c +4 -1
index 7b1633a..96cbd20 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 6 lines · Julia Lawall
Quoted 2 lines · Davidlohr Bueso A.

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

Quoted 33 lines

julia

Quoted 22 lines
Signed-off-by Davidlohr Bueso
---
 drivers/scsi/ch.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/scsi/ch.c +4 -1
index 7b1633a..96cbd20 100644
@@ -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;
--
Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 9 lines · Davidlohr Bueso A.
Quoted 6 lines · Julia Lawall
Quoted 2 lines · Davidlohr Bueso A.

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

author
#4 lore raw
Quoted 8 lines · Julia Lawall
Quoted 6 lines · Davidlohr Bueso A.
Quoted 5 lines · Julia Lawall
Quoted 2 lines · Davidlohr Bueso A.

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

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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 <[email protected]>
---
 drivers/scsi/ch.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
drivers/scsi/ch.c +6 -0
index 7b1633a..bb42ceb 100644
@@ -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;
#5 lore raw
Quoted 34 lines · Davidlohr Bueso A.
Quoted 8 lines · Julia Lawall
Quoted 6 lines · Davidlohr Bueso A.
Quoted 5 lines · Julia Lawall
Quoted 2 lines · Davidlohr Bueso A.

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

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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 <[email protected]>

---
 drivers/scsi/ch.c |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)
drivers/scsi/ch.c +0 -0
index 7b1633a..bb42ceb 100644

drivers/scsi/ch.c ch_readconfig(scsi_changer *ch) · line 358

Quoted 6 lines
drivers/scsi/ch.c +3 -0
@@ -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) ?

Quoted 7 lines
drivers/scsi/ch.c +3 -0
+               return -ENOMEM;
+       }
+
        for (elem = 0; elem < ch->counts[CHET_DT]; elem++) {
                id  = -1;
                lun = 0;
author
#6 lore raw
Quoted 30 lines · Hannes Eder
Quoted 29 lines · Davidlohr Bueso A.
Quoted 8 lines · Julia Lawall
Quoted 6 lines · Davidlohr Bueso A.
Quoted 5 lines · Julia Lawall
Quoted 2 lines · Davidlohr Bueso A.

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

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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 <[email protected]>

---
 drivers/scsi/ch.c |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)
Quoted 13 lines

drivers/scsi/ch.c ch_readconfig(scsi_changer *ch) · line 358

Quoted 11 lines
drivers/scsi/ch.c +3 -0
index 7b1633a..bb42ceb 100644
@@ -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 <[email protected]>
---
 drivers/scsi/ch.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
drivers/scsi/ch.c +6 -0
index 7b1633a..fe11c1d 100644
@@ -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;
maintainer
#7 lore raw
In reply to Davidlohr Bueso A.
Quoted 4 lines · Davidlohr Bueso A.

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 <[email protected]>
Quoted 9 lines
---
 drivers/scsi/ch.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/scsi/ch.c +0 -0
index 7b1633a..96cbd20 100644

drivers/scsi/ch.c ch_readconfig(scsi_changer *ch) · line 358

Quoted 6 lines
drivers/scsi/ch.c +3 -0
@@ -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