This is a cut and paste mistake, sizeof(struct mib_local) was intended
instead of sizeof(struct mib_phy). The call to at76_get_mib() uses
sizeof(struct mib_local) correctly.
The current code works fine because mib_phy structs are larger than
mib_local structs. But we may as well clean it up.
Signed-off-by: Dan Carpenter <[email protected]>
diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index faa8bcb..0bba5ea 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1122,7 +1122,7 @@ exit:
static void at76_dump_mib_local(struct at76_priv *priv)
{
int ret;
- struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
+ struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
if (!m)
return;
On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote:
> Dan Carpenter <[email protected]> writes:
>
> > On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote:
> >> > - ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
> >> > + ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
> >>
> >> Would it be better practice to use sizeof(*m)?
> >>
> >
> > That was my temptation as well... But I decided to make it match
> > with the surrounding code. I'm happy to resend if people want.
>
> IMHO sizeof(*m) is better and I tend to use it.
>
> Related to this: I have a bad habit of sometimes dropping '*' from
> sizeof()? Is there a tool which could spot that?
>
That's what I was working on for Smatch when I sent this patch.
The odd thing is that I can't find any bugs like this in the kernel.
If sizeof(foo) is less than sizeof(*foo), which is probably the
normal case, then these get caught early on in testing.
Still I think people must have done manual audits as well... It
feels too clean to be natural.
regards,
dan carpenter
On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote:
> > - ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
> > + ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
>
> Would it be better practice to use sizeof(*m)?
>
That was my temptation as well... But I decided to make it match
with the surrounding code. I'm happy to resend if people want.
regards,
dan carpenter
This is a cut and paste mistake, sizeof(struct mib_local) was intended
instead of sizeof(struct mib_phy). The call to at76_get_mib() uses
sizeof(struct mib_local) correctly, although I changed that to
sizeof(*m) for style reasons after discussion with some of the wireless
maintainers.
The current code works fine because mib_phy structs are larger than
mib_local structs. But we may as well clean it up.
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: use sizeof(*m) instead of sizeof(struct mib_local).
diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index faa8bcb..3036c0b 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1122,12 +1122,12 @@ exit:
static void at76_dump_mib_local(struct at76_priv *priv)
{
int ret;
- struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
+ struct mib_local *m = kmalloc(sizeof(*m), GFP_KERNEL);
if (!m)
return;
- ret = at76_get_mib(priv->udev, MIB_LOCAL, m, sizeof(struct mib_local));
+ ret = at76_get_mib(priv->udev, MIB_LOCAL, m, sizeof(*m));
if (ret < 0) {
wiphy_err(priv->hw->wiphy,
"at76_get_mib (LOCAL) failed: %d\n", ret);
On Sat, 21 Apr 2012, Dan Carpenter wrote:
> On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote:
>> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it
>> looks like sizeof(x) is coincidentally the same as the size that is
>> wanted. Two cases that look like they could have some noticible
>> effect are:
>>
>> arch/xtensa/platforms/iss/network.c, line 789
>> drivers/block/cciss.c, line 4211
>>
>
> Clever. You'd need to restrict it to places where x was a pointer.
Yes, I didn't put the whole thing:
@r expression@
expression *x;
position p;
@@
x@p = <+... sizeof(x) ...+>
julia
> That's better than my check which was specific to kmalloc(). (So
> uh... I'm going to rewrite mine as well to be more generic. :P)
>
> regards,
> dan carpenter
>
>
On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote:
> On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote:
> > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it
> > looks like sizeof(x) is coincidentally the same as the size that is
> > wanted. Two cases that look like they could have some noticible
> > effect are:
> >
> > arch/xtensa/platforms/iss/network.c, line 789
> > drivers/block/cciss.c, line 4211
> >
>
> Clever. You'd need to restrict it to places where x was a pointer.
> That's better than my check which was specific to kmalloc(). (So
> uh... I'm going to rewrite mine as well to be more generic. :P)
>
Hm... Smatch is not really the right tool here. By the time Sparse
gives you the sizeof(foo) information, it just looks like a number
8.
I hacked up Sparse a bit so it works for simple expressions which
are one token in from the c tokenizer. So:
foo = kmalloc(sizeof(foo), GFP_KERNEL); => error.
foo->bar = kmalloc(sizeof(foo->bar), GFP_KERNEL); => tricky.
It's not ideal. Coccinelle is better for this.
regards,
dan carpenter
Dan Carpenter <[email protected]> writes:
> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote:
>> > - struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
>> > + struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
>>
>> Would it be better practice to use sizeof(*m)?
>>
>
> That was my temptation as well... But I decided to make it match
> with the surrounding code. I'm happy to resend if people want.
IMHO sizeof(*m) is better and I tend to use it.
Related to this: I have a bad habit of sometimes dropping '*' from
sizeof()? Is there a tool which could spot that?
--
Kalle Valo
On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote:
> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it
> looks like sizeof(x) is coincidentally the same as the size that is
> wanted. Two cases that look like they could have some noticible
> effect are:
>
> arch/xtensa/platforms/iss/network.c, line 789
> drivers/block/cciss.c, line 4211
>
Clever. You'd need to restrict it to places where x was a pointer.
That's better than my check which was specific to kmalloc(). (So
uh... I'm going to rewrite mine as well to be more generic. :P)
regards,
dan carpenter
On Sat, 21 Apr 2012, Dan Carpenter wrote:
> On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote:
>> On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote:
>>> Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it
>>> looks like sizeof(x) is coincidentally the same as the size that is
>>> wanted. Two cases that look like they could have some noticible
>>> effect are:
>>>
>>> arch/xtensa/platforms/iss/network.c, line 789
>>> drivers/block/cciss.c, line 4211
>>>
>>
>> Clever. You'd need to restrict it to places where x was a pointer.
>> That's better than my check which was specific to kmalloc(). (So
>> uh... I'm going to rewrite mine as well to be more generic. :P)
>>
>
> Hm... Smatch is not really the right tool here. By the time Sparse
> gives you the sizeof(foo) information, it just looks like a number
> 8.
>
> I hacked up Sparse a bit so it works for simple expressions which
> are one token in from the c tokenizer. So:
>
> foo = kmalloc(sizeof(foo), GFP_KERNEL); => error.
> foo->bar = kmalloc(sizeof(foo->bar), GFP_KERNEL); => tricky.
>
> It's not ideal. Coccinelle is better for this.
On the other hand, Coccinelle has no idea what the size is, so it doesn't
know how important the problem is.
julia
On Sat, 21 Apr 2012, Dan Carpenter wrote:
> On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote:
>> Dan Carpenter <[email protected]> writes:
>>
>>> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote:
>>>>> - ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
>>>>> + ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
>>>>
>>>> Would it be better practice to use sizeof(*m)?
>>>>
>>>
>>> That was my temptation as well... But I decided to make it match
>>> with the surrounding code. I'm happy to resend if people want.
>>
>> IMHO sizeof(*m) is better and I tend to use it.
>>
>> Related to this: I have a bad habit of sometimes dropping '*' from
>> sizeof()? Is there a tool which could spot that?
>>
>
> That's what I was working on for Smatch when I sent this patch.
>
> The odd thing is that I can't find any bugs like this in the kernel.
> If sizeof(foo) is less than sizeof(*foo), which is probably the
> normal case, then these get caught early on in testing.
>
> Still I think people must have done manual audits as well... It
> feels too clean to be natural.
Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it looks
like sizeof(x) is coincidentally the same as the size that is wanted. Two
cases that look like they could have some noticible effect are:
arch/xtensa/platforms/iss/network.c, line 789
drivers/block/cciss.c, line 4211
I will send patches for those two.
julia
On Sat, 21 Apr 2012, Dan Carpenter wrote:
> On Fri, Apr 20, 2012 at 09:14:44PM +0300, Kalle Valo wrote:
>> Dan Carpenter <[email protected]> writes:
>>
>>> On Fri, Apr 20, 2012 at 06:57:00PM +1000, Julian Calaby wrote:
>>>>> - ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
>>>>> + ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
>>>>
>>>> Would it be better practice to use sizeof(*m)?
>>>>
>>>
>>> That was my temptation as well... But I decided to make it match
>>> with the surrounding code. I'm happy to resend if people want.
>>
>> IMHO sizeof(*m) is better and I tend to use it.
>>
>> Related to this: I have a bad habit of sometimes dropping '*' from
>> sizeof()? Is there a tool which could spot that?
>>
>
> That's what I was working on for Smatch when I sent this patch.
>
> The odd thing is that I can't find any bugs like this in the kernel.
> If sizeof(foo) is less than sizeof(*foo), which is probably the
> normal case, then these get caught early on in testing.
>
> Still I think people must have done manual audits as well... It
> feels too clean to be natural.
I sent some patches with respect to this. But that was probably around a
year ago.
julia
On Sat, Apr 21, 2012 at 05:51:41PM +0300, Dan Carpenter wrote:
> On Sat, Apr 21, 2012 at 03:51:44PM +0200, Julia Lawall wrote:
> > Looking for x = ... sizeof(x) ... I get 9 reports. In most cases it
> > looks like sizeof(x) is coincidentally the same as the size that is
> > wanted. Two cases that look like they could have some noticible
> > effect are:
> >
> > arch/xtensa/platforms/iss/network.c, line 789
> > drivers/block/cciss.c, line 4211
> >
>
> Clever. You'd need to restrict it to places where x was a pointer.
> That's better than my check which was specific to kmalloc(). (So
> uh... I'm going to rewrite mine as well to be more generic. :P)
>
The other thing would be to look for places that do:
func(x, sizeof(x);
Of course, you've found a lot of memset()/memcpy() bugs like that in
the past, but probably it could be made more generic so it checks
every function.
regards,
dan carpenter
Hi Dan,
On Fri, Apr 20, 2012 at 16:47, Dan Carpenter <[email protected]> wrote:
> This is a cut and paste mistake, sizeof(struct mib_local) was intended
> instead of sizeof(struct mib_phy). ?The call to at76_get_mib() uses
> sizeof(struct mib_local) correctly.
>
> The current code works fine because mib_phy structs are larger than
> mib_local structs. ?But we may as well clean it up.
>
> Signed-off-by: Dan Carpenter <[email protected]>
>
> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
> index faa8bcb..0bba5ea 100644
> --- a/drivers/net/wireless/at76c50x-usb.c
> +++ b/drivers/net/wireless/at76c50x-usb.c
> @@ -1122,7 +1122,7 @@ exit:
> ?static void at76_dump_mib_local(struct at76_priv *priv)
> ?{
> ? ? ? ?int ret;
> - ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_phy), GFP_KERNEL);
> + ? ? ? struct mib_local *m = kmalloc(sizeof(struct mib_local), GFP_KERNEL);
Would it be better practice to use sizeof(*m)?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/