If we don't allocate "arr" then the cleanup path will dereference it and
oops.
Signed-off-by: Dan Carpenter <[email protected]>
diff --git a/drivers/fmc/fmc-sdb.c b/drivers/fmc/fmc-sdb.c
index 74fb326..79adc39 100644
--- a/drivers/fmc/fmc-sdb.c
+++ b/drivers/fmc/fmc-sdb.c
@@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,
onew = __sdb_rd(fmc, sdb_addr + 4, convert);
n = __be16_to_cpu(*(uint16_t *)&onew);
arr = kzalloc(sizeof(*arr), GFP_KERNEL);
- if (arr) {
- arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
- arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
- }
- if (!arr || !arr->record || !arr->subtree) {
+ if (!arr)
+ return ERR_PTR(-ENOMEM);
+ arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
+ arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
+ if (!arr->record || !arr->subtree) {
kfree(arr->record);
kfree(arr->subtree);
kfree(arr);
return ERR_PTR(-ENOMEM);
}
+
arr->len = n;
arr->level = level;
arr->fmc = fmc;
> If we don't allocate "arr" then the cleanup path will dereference it and
> oops.
You are right, thanks (acked).
How is the procedure here? I don't have my own git tree on
kernel.org for pull requests. Can this go through the janitors?
(if it makes sense, I can try the procedure to have a tree, but last
time I checked it was strictly denied to anyone).
thanks
/alessandro
On Wed, Jun 19, 2013 at 05:25:28PM +0200, Alessandro Rubini wrote:
> > If we don't allocate "arr" then the cleanup path will dereference it and
> > oops.
>
> You are right, thanks (acked).
>
> How is the procedure here? I don't have my own git tree on
> kernel.org for pull requests. Can this go through the janitors?
>
> (if it makes sense, I can try the procedure to have a tree, but last
> time I checked it was strictly denied to anyone).
Apparently these are going through Greg K-H. I'll resend, with Greg
CC'd so he can pick it up from the mailing list.
Could you add an entry to the MAINTAINERS file so that Greg will be
CC'd automatically using get_maintainer.pl? Is there a dedicated
list for FMC development? That would go in the MAINTAINERS file as
well.
regards,
dan carpenter
If we don't allocate "arr" then the cleanup path will dereference it and
oops.
Signed-off-by: Dan Carpenter <[email protected]>
Acked-by: Alessandro Rubini <[email protected]>
diff --git a/drivers/fmc/fmc-sdb.c b/drivers/fmc/fmc-sdb.c
index 74fb326..79adc39 100644
--- a/drivers/fmc/fmc-sdb.c
+++ b/drivers/fmc/fmc-sdb.c
@@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,
onew = __sdb_rd(fmc, sdb_addr + 4, convert);
n = __be16_to_cpu(*(uint16_t *)&onew);
arr = kzalloc(sizeof(*arr), GFP_KERNEL);
- if (arr) {
- arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
- arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
- }
- if (!arr || !arr->record || !arr->subtree) {
+ if (!arr)
+ return ERR_PTR(-ENOMEM);
+ arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
+ arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
+ if (!arr->record || !arr->subtree) {
kfree(arr->record);
kfree(arr->subtree);
kfree(arr);
return ERR_PTR(-ENOMEM);
}
+
arr->len = n;
arr->level = level;
arr->fmc = fmc;
On Wed, 2013-06-19 at 19:01 +0300, Dan Carpenter wrote:
> If we don't allocate "arr" then the cleanup path will dereference it and
> oops.
[]
> diff --git a/drivers/fmc/fmc-sdb.c b/drivers/fmc/fmc-sdb.c
[]
> @@ -46,16 +46,17 @@ static struct sdb_array *__fmc_scan_sdb_tree(struct fmc_device *fmc,
[]
> - arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
> - arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
[]
> + arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
> + arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
n comes from the hardware no?
Maybe make these kcalloc too.
> Apparently these are going through Greg K-H. I'll resend, with Greg
> CC'd so he can pick it up from the mailing list.
>
> Could you add an entry to the MAINTAINERS file so that Greg will be
> CC'd automatically using get_maintainer.pl?
Ok. Added to my todo list.
> Is there a dedicated list for FMC development? That would go in the
> MAINTAINERS file as well.
No, there is no list. Or "not yet". The project was born externally
(on ohwr.org), mainly within the "white rabbit" research group. If it
makes sense, we can have a public list, I'm all for it.
thanks
/alessandro
>> + arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
>> + arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
> n comes from the hardware no?
Yes. Length of hardware description array.
> Maybe make these kcalloc too.
I'm not a fan of kcalloc. I think it removes readability. I remeber
kernel patches to swap the arguments, because people get them wrong.
Even Kernighan said it was a design error (in "the practice of
programming"). That said, I'm not the leader here.
thanks
/alessandro
On Wed, 2013-06-19 at 19:57 +0200, Alessandro Rubini wrote:
> >> + arr->record = kzalloc(sizeof(arr->record[0]) * n, GFP_KERNEL);
> >> + arr->subtree = kzalloc(sizeof(arr->subtree[0]) * n, GFP_KERNEL);
>
> > n comes from the hardware no?
>
> Yes. Length of hardware description array.
>
> > Maybe make these kcalloc too.
>
> I'm not a fan of kcalloc. I think it removes readability. I remeber
> kernel patches to swap the arguments, because people get them wrong.
kcalloc's sole benefit is multiplication overflow protection.
If sizes are small and known, kcalloc isn't particularly useful.
Those kcalloc arg swap patches were just for style with no net effect.
> Even Kernighan said it was a design error (in "the practice of
> programming"). That said, I'm not the leader here.
I think the real design pattern error was realloc()
Anyway, no worries...
On Wed, Jun 19, 2013 at 07:57:58PM +0200, Alessandro Rubini wrote:
> I'm not a fan of kcalloc. I think it removes readability.
Ok. We'll go with my original patch then.
regards,
dan carpenter