* [PATCH] drivers/scsi: Check NULL for kmalloc() return @ 2009-08-07 18:39 Davidlohr Bueso A. 2009-08-07 18:54 ` Julia Lawall 2009-08-07 19:54 ` James Bottomley 0 siblings, 2 replies; 8+ messages in thread From: Davidlohr Bueso A. @ 2009-08-07 18:39 UTC (permalink / raw) To: linux-scsi, kernel-janitors 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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 18:39 [PATCH] drivers/scsi: Check NULL for kmalloc() return Davidlohr Bueso A. @ 2009-08-07 18:54 ` Julia Lawall 2009-08-07 19:44 ` Davidlohr Bueso A. 2009-08-07 19:54 ` James Bottomley 1 sibling, 1 reply; 8+ messages in thread From: Julia Lawall @ 2009-08-07 18:54 UTC (permalink / raw) To: Davidlohr Bueso A.; +Cc: linux-scsi, kernel-janitors 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 18:54 ` Julia Lawall @ 2009-08-07 19:44 ` Davidlohr Bueso A. 2009-08-07 19:56 ` Julia Lawall 0 siblings, 1 reply; 8+ messages in thread From: Davidlohr Bueso A. @ 2009-08-07 19:44 UTC (permalink / raw) To: Julia Lawall, kraxel; +Cc: linux-scsi, kernel-janitors 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 19:44 ` Davidlohr Bueso A. @ 2009-08-07 19:56 ` Julia Lawall 2009-08-07 20:17 ` Davidlohr Bueso A. 0 siblings, 1 reply; 8+ messages in thread From: Julia Lawall @ 2009-08-07 19:56 UTC (permalink / raw) To: Davidlohr Bueso A.; +Cc: kraxel, linux-scsi, kernel-janitors 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 19:56 ` Julia Lawall @ 2009-08-07 20:17 ` Davidlohr Bueso A. 2009-08-07 20:33 ` Hannes Eder 0 siblings, 1 reply; 8+ messages in thread From: Davidlohr Bueso A. @ 2009-08-07 20:17 UTC (permalink / raw) To: Julia Lawall; +Cc: kraxel, linux-scsi, kernel-janitors 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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 20:17 ` Davidlohr Bueso A. @ 2009-08-07 20:33 ` Hannes Eder 2009-08-07 20:42 ` Davidlohr Bueso A. 0 siblings, 1 reply; 8+ messages in thread From: Hannes Eder @ 2009-08-07 20:33 UTC (permalink / raw) To: Davidlohr Bueso A.; +Cc: Julia Lawall, kraxel, linux-scsi, kernel-janitors 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; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 20:33 ` Hannes Eder @ 2009-08-07 20:42 ` Davidlohr Bueso A. 0 siblings, 0 replies; 8+ messages in thread From: Davidlohr Bueso A. @ 2009-08-07 20:42 UTC (permalink / raw) To: Hannes Eder; +Cc: Julia Lawall, kraxel, linux-scsi, kernel-janitors 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; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/scsi: Check NULL for kmalloc() return 2009-08-07 18:39 [PATCH] drivers/scsi: Check NULL for kmalloc() return Davidlohr Bueso A. 2009-08-07 18:54 ` Julia Lawall @ 2009-08-07 19:54 ` James Bottomley 1 sibling, 0 replies; 8+ messages in thread From: James Bottomley @ 2009-08-07 19:54 UTC (permalink / raw) To: Davidlohr Bueso A.; +Cc: linux-scsi, kernel-janitors 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-08-07 20:42 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-07 18:39 [PATCH] drivers/scsi: Check NULL for kmalloc() return Davidlohr Bueso A. 2009-08-07 18:54 ` Julia Lawall 2009-08-07 19:44 ` Davidlohr Bueso A. 2009-08-07 19:56 ` Julia Lawall 2009-08-07 20:17 ` Davidlohr Bueso A. 2009-08-07 20:33 ` Hannes Eder 2009-08-07 20:42 ` Davidlohr Bueso A. 2009-08-07 19:54 ` James Bottomley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox