From: Markus Elfring <[email protected]>
Date: Fri, 9 Dec 2016 19:09:13 +0100
The function "kmalloc" was called in one case by the function "sb_equal"
without checking immediately if it failed.
This issue was detected by using the Coccinelle software.
Perform the desired memory allocation (and release at the end)
by a single function call instead.
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/md/md.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index b088668269b0..86caf2536255 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -843,15 +843,12 @@ static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
int ret;
mdp_super_t *tmp1, *tmp2;
- tmp1 = kmalloc(sizeof(*tmp1),GFP_KERNEL);
- tmp2 = kmalloc(sizeof(*tmp2),GFP_KERNEL);
-
- if (!tmp1 || !tmp2) {
- ret = 0;
- goto abort;
- }
+ tmp1 = kmalloc(2 * sizeof(*tmp1), GFP_KERNEL);
+ if (!tmp1)
+ return 0;
*tmp1 = *sb1;
+ tmp2 = tmp1 + 1;
*tmp2 = *sb2;
/*
@@ -861,9 +858,7 @@ static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
tmp2->nr_disks = 0;
ret = (memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * 4) == 0);
-abort:
kfree(tmp1);
- kfree(tmp2);
return ret;
}
--
2.11.0
On Fri, 2016-12-09 at 19:30 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Fri, 9 Dec 2016 19:09:13 +0100
>
> The function "kmalloc" was called in one case by the function "sb_equal"
> without checking immediately if it failed.
> This issue was detected by using the Coccinelle software.
>
> Perform the desired memory allocation (and release at the end)
> by a single function call instead.
>
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Making a change does not mean fixes.
There's nothing particularly _wrong_ with the code as-is.
2 kmemdup calls might make the code more obvious.
There's a small optimization possible in that only the
first MB_SB_GENERIC_CONSTANT_WORDS of the struct are
actually compared. Alloc and copy of both entire structs
is inefficient and unnecessary.
Perhaps something like the below would be marginally
better/faster, but the whole thing is dubious.
static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
{
int ret;
void *tmp1, *tmp2;
tmp1 = kmemdup(sb1, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
tmp2 = kmemdup(sb2, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
if (!tmp1 || !tmp2) {
ret = 0;
goto out;
}
/*
* nr_disks is not constant
*/
((mdp_super_t *)tmp1)->nr_disks = 0;
((mdp_super_t *)tmp2)->nr_disks = 0;
ret = memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32)) == 0;
out:
kfree(tmp1);
kfree(tmp2);
return ret;
}
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/md/md.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index b088668269b0..86caf2536255 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -843,15 +843,12 @@ static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> int ret;
> mdp_super_t *tmp1, *tmp2;
>
> - tmp1 = kmalloc(sizeof(*tmp1),GFP_KERNEL);
> - tmp2 = kmalloc(sizeof(*tmp2),GFP_KERNEL);
> -
> - if (!tmp1 || !tmp2) {
> - ret = 0;
> - goto abort;
> - }
> + tmp1 = kmalloc(2 * sizeof(*tmp1), GFP_KERNEL);
> + if (!tmp1)
> + return 0;
>
> *tmp1 = *sb1;
> + tmp2 = tmp1 + 1;
> *tmp2 = *sb2;
>
> /*
> @@ -861,9 +858,7 @@ static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> tmp2->nr_disks = 0;
>
> ret = (memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * 4) == 0);
> -abort:
> kfree(tmp1);
> - kfree(tmp2);
> return ret;
> }
On 09.12.2016 19:30, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Fri, 9 Dec 2016 19:09:13 +0100
>
> The function "kmalloc" was called in one case by the function "sb_equal"
> without checking immediately if it failed.
Err, your patch actually *replaces* the check. So where did you get the
idea from that it is not checked immediately?
[...]
> - tmp1 = kmalloc(sizeof(*tmp1),GFP_KERNEL);
> - tmp2 = kmalloc(sizeof(*tmp2),GFP_KERNEL);
> -
> - if (!tmp1 || !tmp2) {
> - ret = 0;
> - goto abort;
> - }
This is not immediately?
Bernd
> So where did you get the idea from that it is not checked immediately?
Is another variable assignment performed so far before the return value
is checked from a previous function call?
Regards,
Markus
> tmp1 = kmemdup(sb1, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
Is a function available in the Linux programming interface which would duplicate
the beginning of two array elements in a single call directly?
Regards,
Markus
On Fri, 2016-12-09 at 21:05 +0100, SF Markus Elfring wrote:
> > tmp1 = kmemdup(sb1, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
>
> Is a function available in the Linux programming interface which would duplicate
> the beginning of two array elements in a single call directly?
No. If there were, there would be a good argument to remove it.
On 09.12.2016 20:54, SF Markus Elfring wrote:
>> So where did you get the idea from that it is not checked immediately?
>
> Is another variable assignment performed so far before the return value
> is checked from a previous function call?
Irrelevant, the variable is not used before checking it.
On Fri, Dec 09, 2016 at 11:05:14AM -0800, Joe Perches wrote:
> On Fri, 2016-12-09 at 19:30 +0100, SF Markus Elfring wrote:
> > From: Markus Elfring <[email protected]>
> > Date: Fri, 9 Dec 2016 19:09:13 +0100
> >
> > The function "kmalloc" was called in one case by the function "sb_equal"
> > without checking immediately if it failed.
> > This issue was detected by using the Coccinelle software.
> >
> > Perform the desired memory allocation (and release at the end)
> > by a single function call instead.
> >
> > Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
>
> Making a change does not mean fixes.
>
> There's nothing particularly _wrong_ with the code as-is.
>
> 2 kmemdup calls might make the code more obvious.
>
> There's a small optimization possible in that only the
> first MB_SB_GENERIC_CONSTANT_WORDS of the struct are
> actually compared. Alloc and copy of both entire structs
> is inefficient and unnecessary.
>
> Perhaps something like the below would be marginally
> better/faster, but the whole thing is dubious.
>
> static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> {
> int ret;
> void *tmp1, *tmp2;
>
> tmp1 = kmemdup(sb1, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
> tmp2 = kmemdup(sb2, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
>
> if (!tmp1 || !tmp2) {
> ret = 0;
> goto out;
> }
>
> /*
> * nr_disks is not constant
> */
> ((mdp_super_t *)tmp1)->nr_disks = 0;
> ((mdp_super_t *)tmp2)->nr_disks = 0;
>
> ret = memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32)) == 0;
>
> out:
> kfree(tmp1);
> kfree(tmp2);
> return ret;
> }
May I politely inquire if either of you has actually bothered to read the
code and figure out what it does? This is grotesque...
For really slow: we have two objects. We want to check if anything in the
128-byte chunks in their beginnings other than one 32bit field happens to be
different. For that we
* allocate two 128-byte pieces of memory
* *copy* our objects into those
* forcibly zero the field in question in both of those copies
* compare the fuckers
* free them
And you two are discussing whether it's better to combine allocations of those
copies into a single 256-byte allocation? Really? _IF_ it is a hot path,
the obvious optimization would be to avoid copying that crap in the first
place - simply by
return memcmp(sb1, sb2, offsetof(mdp_super_t, nr_disks)) ||
memcmp(&sb1->nr_disks + 1, &sb2->nr_disks + 1,
MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32) -
offsetof(mdp_super_t, nr_disks) - 4);
If it is _not_ a hot path, why bother with it at all?
On Fri, 2016-12-09 at 21:30 +0000, Al Viro wrote:
> On Fri, Dec 09, 2016 at 11:05:14AM -0800, Joe Perches wrote:
> > On Fri, 2016-12-09 at 19:30 +0100, SF Markus Elfring wrote:
> > > From: Markus Elfring <[email protected]>
> > > Date: Fri, 9 Dec 2016 19:09:13 +0100
> > >
> > > The function "kmalloc" was called in one case by the function "sb_equal"
> > > without checking immediately if it failed.
> > > This issue was detected by using the Coccinelle software.
> > >
> > > Perform the desired memory allocation (and release at the end)
> > > by a single function call instead.
> > >
> > > Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
> >
> > Making a change does not mean fixes.
> >
> > There's nothing particularly _wrong_ with the code as-is.
> >
> > 2 kmemdup calls might make the code more obvious.
> >
> > There's a small optimization possible in that only the
> > first MB_SB_GENERIC_CONSTANT_WORDS of the struct are
> > actually compared. Alloc and copy of both entire structs
> > is inefficient and unnecessary.
> >
> > Perhaps something like the below would be marginally
> > better/faster, but the whole thing is dubious.
> >
> > static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> > {
> > int ret;
> > void *tmp1, *tmp2;
> >
> > tmp1 = kmemdup(sb1, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
> > tmp2 = kmemdup(sb2, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32), GFP_KERNEL);
> >
> > if (!tmp1 || !tmp2) {
> > ret = 0;
> > goto out;
> > }
> >
> > /*
> > * nr_disks is not constant
> > */
> > ((mdp_super_t *)tmp1)->nr_disks = 0;
> > ((mdp_super_t *)tmp2)->nr_disks = 0;
> >
> > ret = memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32)) == 0;
> >
> > out:
> > kfree(tmp1);
> > kfree(tmp2);
> > return ret;
> > }
>
> May I politely inquire if either of you has actually bothered to read the
> code and figure out what it does? This is grotesque...
>
> For really slow: we have two objects. We want to check if anything in the
> 128-byte chunks in their beginnings other than one 32bit field happens to be
> different. For that we
> * allocate two 128-byte pieces of memory
> * *copy* our objects into those
> * forcibly zero the field in question in both of those copies
> * compare the fuckers
> * free them
>
> And you two are discussing whether it's better to combine allocations of those
> copies into a single 256-byte allocation? Really?
No. May I suggest you read my suggestion?
At no point did I suggest a single allocation.
I think the single allocation is silly and just
makes the code harder to read.
> _IF_ it is a hot path,
> the obvious optimization would be to avoid copying that crap in the first
> place - simply by
> return memcmp(sb1, sb2, offsetof(mdp_super_t, nr_disks)) ||
> memcmp(&sb1->nr_disks + 1, &sb2->nr_disks + 1,
> MD_SB_GENERIC_CONSTANT_WORDS * sizeof(__u32) -
> offsetof(mdp_super_t, nr_disks) - 4);
That's all true, but Markus has enough trouble reading simple
code without trying to explain to him what offsetof does.
btw: the "- 4" should be " - sizeof(__u32)" just for consistency
with the line above it.
> If it is _not_ a hot path, why bother with it at all?
exactly.
> Irrelevant, the variable is not used before checking it.
* Will it be more appropriate to attempt another memory allocation only if
the previous one succeeded already?
* Can it be a bit more efficient to duplicate only the required data
in a single function call before?
Regards,
Markus
On 09.12.2016 22:58, SF Markus Elfring wrote:
>> Irrelevant, the variable is not used before checking it.
>
> * Will it be more appropriate to attempt another memory allocation only if
> the previous one succeeded already?
>
> * Can it be a bit more efficient to duplicate only the required data
> in a single function call before?
How many memory allocations do you expect to fail?