sparse vs. skbuff.h

sparse vs. skbuff.h

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

commit 14d18a81b5171d4433e41129619c75748b4f4d26
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Thu Oct 29 00:10:37 2009 +0000

    net: fix kmemcheck annotations


broke sparse endian checks on everything that includes skbuff.h because
the first and only (because it's an error) thing sparse now reports is
this:

include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: sparse vs. skbuff.h

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
> commit 14d18a81b5171d4433e41129619c75748b4f4d26
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Thu Oct 29 00:10:37 2009 +0000
> 
>     net: fix kmemcheck annotations
> 
> 
> broke sparse endian checks on everything that includes skbuff.h because
> the first and only (because it's an error) thing sparse now reports is
> this:
> 
> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.

Simply changing from
	__be16 protocol:16;
to
	__be16 protocol;

but leaving it inside the kmemcheck annotation seems to do the right
thing. Except of course that kmemcheck will not properly check it now.
Maybe those annotations should simply be made to have no impact on
struct padding instead?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: sparse vs. skbuff.h

[Eric Dumazet]

Johannes Berg a écrit :
> On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
>> commit 14d18a81b5171d4433e41129619c75748b4f4d26
>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>> Date:   Thu Oct 29 00:10:37 2009 +0000
>>
>>     net: fix kmemcheck annotations
>>
>>
>> broke sparse endian checks on everything that includes skbuff.h because
>> the first and only (because it's an error) thing sparse now reports is
>> this:
>>
>> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.
> 
> Simply changing from
> 	__be16 protocol:16;
> to
> 	__be16 protocol;
> 
> but leaving it inside the kmemcheck annotation seems to do the right
> thing. Except of course that kmemcheck will not properly check it now.
> Maybe those annotations should simply be made to have no impact on
> struct padding instead?
> 

Hmm, I have really no idea of what is the right way to fix this stuff.

Last time I did adding a non bitfield element inside the begin/end annotations,
I was flamed, because a bitfield is a bitfield, not a char/short

http://www.spinics.net/lists/netdev/msg108803.html

Now sparse is complaining... What will be the next story ?

Re: sparse vs. skbuff.h

[Vegard Nossum]

2009/11/16 Eric Dumazet <eric.dumazet@gmail.com>:
> Johannes Berg a écrit :
>> On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
>>> commit 14d18a81b5171d4433e41129619c75748b4f4d26
>>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date:   Thu Oct 29 00:10:37 2009 +0000
>>>
>>>     net: fix kmemcheck annotations
>>>
>>>
>>> broke sparse endian checks on everything that includes skbuff.h because
>>> the first and only (because it's an error) thing sparse now reports is
>>> this:
>>>
>>> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.
>>
>> Simply changing from
>>       __be16 protocol:16;
>> to
>>       __be16 protocol;
>>
>> but leaving it inside the kmemcheck annotation seems to do the right
>> thing. Except of course that kmemcheck will not properly check it now.
>> Maybe those annotations should simply be made to have no impact on
>> struct padding instead?
>>
>
> Hmm, I have really no idea of what is the right way to fix this stuff.
>
> Last time I did adding a non bitfield element inside the begin/end annotations,
> I was flamed, because a bitfield is a bitfield, not a char/short
>
> http://www.spinics.net/lists/netdev/msg108803.html
>
> Now sparse is complaining... What will be the next story ?

If by "I was flamed" you are referring to my reply:

http://www.spinics.net/lists/netdev/msg108825.html

then I am really sorry, because I had no intentions to insult you. In
fact, I am grateful that you are finding bugs and telling me about
them But I should also be allowed to disagree with a patch if I truly
believe it is the wrong thing to do.

For the issue in question: If the variable is turned into a
non-bitfield (as Johannes suggested), it would be fine, because now
GCC won't emit masking operations (AND, OR) when initializing it, but
a regular MOV. Also, the struct annotations do not by themselves do
anything, but they are used by kmemcheck_annotate_bitfield().

In other words, I think the right thing to do is to turn it into a
non-bitfield and move it _outside_ the bitfield annotation. Johannes,
can you make the patch and let us have a look? In the meantime I will
submit the patch that fixes the extraneous field padding in
KMEMCHECK=n kernels.


Vegard

Re: sparse vs. skbuff.h

[Eric Dumazet]

Vegard Nossum a écrit :
> 2009/11/16 Eric Dumazet <eric.dumazet@gmail.com>:
>> Johannes Berg a écrit :
>>> On Mon, 2009-11-16 at 20:21 +0100, Johannes Berg wrote:
>>>> commit 14d18a81b5171d4433e41129619c75748b4f4d26
>>>> Author: Eric Dumazet <eric.dumazet@gmail.com>
>>>> Date:   Thu Oct 29 00:10:37 2009 +0000
>>>>
>>>>     net: fix kmemcheck annotations
>>>>
>>>>
>>>> broke sparse endian checks on everything that includes skbuff.h because
>>>> the first and only (because it's an error) thing sparse now reports is
>>>> this:
>>>>
>>>> include/linux/skbuff.h:357:41: error: invalid bitfield specifier for type restricted __be16.
>>> Simply changing from
>>>       __be16 protocol:16;
>>> to
>>>       __be16 protocol;
>>>
>>> but leaving it inside the kmemcheck annotation seems to do the right
>>> thing. Except of course that kmemcheck will not properly check it now.
>>> Maybe those annotations should simply be made to have no impact on
>>> struct padding instead?
>>>
>> Hmm, I have really no idea of what is the right way to fix this stuff.
>>
>> Last time I did adding a non bitfield element inside the begin/end annotations,
>> I was flamed, because a bitfield is a bitfield, not a char/short
>>
>> http://www.spinics.net/lists/netdev/msg108803.html
>>
>> Now sparse is complaining... What will be the next story ?
> 
> If by "I was flamed" you are referring to my reply:
> 
> http://www.spinics.net/lists/netdev/msg108825.html
> 
> then I am really sorry, because I had no intentions to insult you. In
> fact, I am grateful that you are finding bugs and telling me about
> them But I should also be allowed to disagree with a patch if I truly
> believe it is the wrong thing to do.

Oh well, maybe flamed is not the right word Vegard, and I did not feel
being insulted at all ! Sorry if the tone of my mail was misleading.

> 
> For the issue in question: If the variable is turned into a
> non-bitfield (as Johannes suggested), it would be fine, because now
> GCC won't emit masking operations (AND, OR) when initializing it, but
> a regular MOV. Also, the struct annotations do not by themselves do
> anything, but they are used by kmemcheck_annotate_bitfield().
> 
> In other words, I think the right thing to do is to turn it into a
> non-bitfield and move it _outside_ the bitfield annotation. Johannes,
> can you make the patch and let us have a look? In the meantime I will
> submit the patch that fixes the extraneous field padding in
> KMEMCHECK=n kernels.
> 

Thanks