[RFC][PATCH] bitfields API

[RFC][PATCH] bitfields API

[Vegard Nossum]

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

Hi,

kmemcheck is the kernel patch that detects use of uninitialized memory.

kmemcheck reports eagerly, that is, any load of uninitialized memory
will be reported, even though the bits in question will be thrown away
later (before they are used in making a decision).

One problem that we have encountered (which generates a false positive
report) is that of bitfield accesses. In particular, whenever a bit of
a bitfield is set (or cleared), the whole byte may be loaded before the
specific bit is toggled. If the bitfield has not been accessed prior to
this, it will of course load the uninitialized byte and report this to
the user.

One solution to the problem is to tell the compiler that the memory
location in question is in fact uninitialized. This means that the
compiler can optimize out the initial load (since it will be unused in
either case), and no warning will be given by kmemcheck.

One way to tell this to the compiler is to set all the members of the
bitfield at once. We could do this unconditionally, although it adds
some overhead on certain architectures, or we can do it for certain
architectures only. My proposal is the attached patch -- a bitfields
"API". Please see the patch description for a thorough explanation.

(Please note that the patches do nothing unless the architecture is
explicit about wanting the functionality. On i386, the second patch in
the series would actually save 7 bytes, while on sparc it would take
about 48 bytes _more_.)

(The alternative to the patch is to explicitly initialize all the
fields [including any leftover "padding" fields that must also be
present] to zero at the same time, before copying in the new values.
This seems a little excessive to me, and I believe that a single
bitfield_begin_init() is neater and harder to break than an explicit
[and seemingly meaningless] list of bitfield member initializations).

So:

How do you feel about this patch? It's all about making kmemcheck more
useful... and not much else. Does it have any chance of entering the
kernel along with kmemcheck (when/if that happens)?

Thanks in advance.


Vegard

[-- Attachment #2: 0001-bitfields-add-bitfields-API.patch --]
[-- Type: text/plain, Size: 2267 bytes --]

>From ef0d12868cacb41ed13705fcf28d35fd96d284a8 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Wed, 27 Aug 2008 18:37:16 +0200
Subject: [PATCH] bitfields: add bitfields API

It turns out that there is an inherent conflict between the way GCC
encodes bitfield operations and the way kmemcheck detects uses of
uninitialized memory; since bitfields span one or more bytes,
accessing only a single bit may generate a load of the whole byte.
This happens regardless of whether we actually want to preserve the
contents of the rest of the bitfield or not.

The solution to the problem is to initialize all the fields at once,
which means that GCC can now optimize out the initial load. This can
actually even save a few bytes on certain architectures, although
this is not the main purpose of the patch. On other architectures,
the generated assembly code will be slightly longer; therefore, the
patch should do nothing in those cases.

This new header defines one macro for defining bitfields,
DEFINE_BITFIELD, and one macro for annotating the places where the
bitfield is being initialized, bitfield_begin_init. See the next
patch in the series for an example of how to use these macros.

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 include/linux/bitfield.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index 0000000..2d02ba3
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,26 @@
+#ifndef LINUX_BITFIELD_H
+#define LINUX_BITFIELD_H
+
+#define DEFINE_BITFIELD(type, name, fields...)	\
+	union {					\
+		struct {			\
+			type _fields;		\
+		} name;				\
+		struct {			\
+			type fields;		\
+		};				\
+	};
+
+/*
+ * NOTE: This macro should NOT be used to initialize the bitfield! Its
+ * purpose is to inform the compiler that the bitfield is currently
+ * uninitialized and that the other bits do not have to be preserved on
+ * subsequent changes to the bitfield.
+ */
+#ifdef CONFIG_ARCH_WANT_BITFIELD_INITIALIZERS
+#define bitfield_begin_init(name) ((name)._fields = 0)
+#else
+#define bitfield_begin_init(name)
+#endif
+
+#endif
-- 
1.5.5.1


[-- Attachment #3: 0002-net-use-bitfields-API-in-skbuff.patch --]
[-- Type: text/plain, Size: 1707 bytes --]

>From 39caa17f8bce166a69d9e49643ecb79db95b02f4 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Wed, 27 Aug 2008 18:44:29 +0200
Subject: [PATCH] net: use bitfields API in skbuff

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 include/linux/skbuff.h |    6 ++++--
 net/core/skbuff.c      |    2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9099237..35fe08d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -14,6 +14,7 @@
 #ifndef _LINUX_SKBUFF_H
 #define _LINUX_SKBUFF_H
 
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/compiler.h>
 #include <linux/time.h>
@@ -285,11 +286,12 @@ struct sk_buff {
 		};
 	};
 	__u32			priority;
-	__u8			local_df:1,
+	DEFINE_BITFIELD(__u8,	flags1,
+				local_df:1,
 				cloned:1,
 				ip_summed:2,
 				nohdr:1,
-				nfctinfo:3;
+				nfctinfo:3);
 	__u8			pkt_type:3,
 				fclone:2,
 				ipvs_property:1,
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ca1ccdf..1a2505b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -36,6 +36,7 @@
  *	The functions in this file will not compile correctly with gcc 2.4.x
  */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -438,6 +439,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 	memcpy(new->cb, old->cb, sizeof(old->cb));
 	new->csum_start		= old->csum_start;
 	new->csum_offset	= old->csum_offset;
+	bitfield_begin_init(new->flags1);
 	new->local_df		= old->local_df;
 	new->pkt_type		= old->pkt_type;
 	new->ip_summed		= old->ip_summed;
-- 
1.5.5.1

Re: [RFC][PATCH] bitfields API

[Alexey Dobriyan]

On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
> How do you feel about this patch? It's all about making kmemcheck more
> useful... and not much else. Does it have any chance of entering the
> kernel along with kmemcheck (when/if that happens)?

DEFINE_BITFIELD is horrible.

> @@ -285,11 +286,12 @@ struct sk_buff {
>  		};
>  	};
>  	__u32			priority;
> -	__u8			local_df:1,
> +	DEFINE_BITFIELD(__u8,	flags1,
> +				local_df:1,
>  				cloned:1,
>  				ip_summed:2,
>  				nohdr:1,
> -				nfctinfo:3;
> +				nfctinfo:3);
>  	__u8			pkt_type:3,
>  				fclone:2,
>  				ipvs_property:1,

Re: [RFC][PATCH] bitfields API

[Pekka Enberg]

Hi Alexey,

Alexey Dobriyan wrote:
> On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
>> How do you feel about this patch? It's all about making kmemcheck more
>> useful... and not much else. Does it have any chance of entering the
>> kernel along with kmemcheck (when/if that happens)?
> 
> DEFINE_BITFIELD is horrible.

Heh, heh, one alternative is to have a kmemcheck_memset() thingy that 
unconditionally zeroes bit fields and maybe is a no-op when kmemcheck is 
disabled.

Re: [RFC][PATCH] bitfields API

[Vegard Nossum]

On Thu, Aug 28, 2008 at 8:40 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
>> How do you feel about this patch? It's all about making kmemcheck more
>> useful... and not much else. Does it have any chance of entering the
>> kernel along with kmemcheck (when/if that happens)?
>
> DEFINE_BITFIELD is horrible.
>
>> @@ -285,11 +286,12 @@ struct sk_buff {
>>               };
>>       };
>>       __u32                   priority;
>> -     __u8                    local_df:1,
>> +     DEFINE_BITFIELD(__u8,   flags1,
>> +                             local_df:1,
>>                               cloned:1,
>>                               ip_summed:2,
>>                               nohdr:1,
>> -                             nfctinfo:3;
>> +                             nfctinfo:3);
>>       __u8                    pkt_type:3,
>>                               fclone:2,
>>                               ipvs_property:1,

Ok, that's constructive :-P

Can we skip the type and always assume that it should be __u8/uint8_t?
I read somewhere that bitfields should anyway always be 1 byte wide if
the bitfield should be "portable". Would it help (to make this less
horrible) to omit the type declaration and have just the bitfield
members as arguments to the macro?

Thanks,


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] bitfields API

[Pekka J Enberg]

On Thu, 28 Aug 2008, Vegard Nossum wrote:
> On Thu, Aug 28, 2008 at 8:40 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
> >> How do you feel about this patch? It's all about making kmemcheck more
> >> useful... and not much else. Does it have any chance of entering the
> >> kernel along with kmemcheck (when/if that happens)?
> >
> > DEFINE_BITFIELD is horrible.
> >
> >> @@ -285,11 +286,12 @@ struct sk_buff {
> >>               };
> >>       };
> >>       __u32                   priority;
> >> -     __u8                    local_df:1,
> >> +     DEFINE_BITFIELD(__u8,   flags1,
> >> +                             local_df:1,
> >>                               cloned:1,
> >>                               ip_summed:2,
> >>                               nohdr:1,
> >> -                             nfctinfo:3;
> >> +                             nfctinfo:3);
> >>       __u8                    pkt_type:3,
> >>                               fclone:2,
> >>                               ipvs_property:1,
> 
> Ok, that's constructive :-P
> 
> Can we skip the type and always assume that it should be __u8/uint8_t?
> I read somewhere that bitfields should anyway always be 1 byte wide if
> the bitfield should be "portable". Would it help (to make this less
> horrible) to omit the type declaration and have just the bitfield
> members as arguments to the macro?

Why not do something like this (as suggested by Ingo, I think)? Yeah, the 
macro should go into kmemcheck.h but I don't have a tree handy...

		Pekka

Index: linux-2.6/include/linux/bitfield.h
===================================================================
--- /dev/null
+++ linux-2.6/include/linux/bitfield.h
@@ -0,0 +1,10 @@
+#ifndef __LINUX_BITFIELD_H
+#define __LINUX_BITFIELD_H
+
+#ifdef CONFIG_KMEMCHECK
+#define KMEMCHECK_BIT_FIELD(field) do { field = 0; } while (0)
+#else
+#define KMEMCHECK_BIT_FIELD(field) do { } while (0)
+#endif /* CONFIG_KMEMCHECK */
+
+#endif /* __LINUX_BITFIELD_H */

Index: linux-2.6/net/core/skbuff.c
===================================================================
--- linux-2.6.orig/net/core/skbuff.c
+++ linux-2.6/net/core/skbuff.c
@@ -55,6 +55,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/init.h>
 #include <linux/scatterlist.h>
+#include <linux/bitfield.h>
 
 #include <net/protocol.h>
 #include <net/dst.h>
@@ -209,6 +210,11 @@ struct sk_buff *__alloc_skb(unsigned int
 	skb->data = data;
 	skb_reset_tail_pointer(skb);
 	skb->end = skb->tail + size;
+	KMEMCHECK_BIT_FIELD(skb->local_df);
+	KMEMCHECK_BIT_FIELD(skb->cloned);
+	KMEMCHECK_BIT_FIELD(skb->ip_summed);
+	KMEMCHECK_BIT_FIELD(skb->nohdr);
+	KMEMCHECK_BIT_FIELD(skb->nfctinfo);
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);
 	atomic_set(&shinfo->dataref, 1);

Re: [RFC][PATCH] bitfields API

[Alexey Dobriyan]

On Thu, Aug 28, 2008 at 08:46:43PM +0200, Vegard Nossum wrote:
> On Thu, Aug 28, 2008 at 8:40 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
> >> How do you feel about this patch? It's all about making kmemcheck more
> >> useful... and not much else. Does it have any chance of entering the
> >> kernel along with kmemcheck (when/if that happens)?
> >
> > DEFINE_BITFIELD is horrible.
> >
> >> @@ -285,11 +286,12 @@ struct sk_buff {
> >>               };
> >>       };
> >>       __u32                   priority;
> >> -     __u8                    local_df:1,
> >> +     DEFINE_BITFIELD(__u8,   flags1,
> >> +                             local_df:1,
> >>                               cloned:1,
> >>                               ip_summed:2,
> >>                               nohdr:1,
> >> -                             nfctinfo:3;
> >> +                             nfctinfo:3);
> >>       __u8                    pkt_type:3,
> >>                               fclone:2,
> >>                               ipvs_property:1,
> 
> Ok, that's constructive :-P
> 
> Can we skip the type and always assume that it should be __u8/uint8_t?

Of course, no.

> I read somewhere that bitfields should anyway always be 1 byte wide if
> the bitfield should be "portable".

It should be signed int or unsigned int for maximum portability.

> Would it help (to make this less
> horrible) to omit the type declaration and have just the bitfield
> members as arguments to the macro?

Or you can parse instruction stream a bit more.

Re: [RFC][PATCH] bitfields API

[Pekka Enberg]

Hi Alexey,

Alexey Dobriyan wrote:
>> Would it help (to make this less
>> horrible) to omit the type declaration and have just the bitfield
>> members as arguments to the macro?
> 
> Or you can parse instruction stream a bit more.

Uhm, I'm not sure how that would work out. I mean, we'd need to see if 
parts of the loaded byte are used or not... Doesn't sound practical or 
too pretty.

		Pekka

Re: [RFC][PATCH] bitfields API

[Vegard Nossum]

On Thu, Aug 28, 2008 at 9:05 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Thu, Aug 28, 2008 at 08:46:43PM +0200, Vegard Nossum wrote:
>> On Thu, Aug 28, 2008 at 8:40 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> > On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
>> >> How do you feel about this patch? It's all about making kmemcheck more
>> >> useful... and not much else. Does it have any chance of entering the
>> >> kernel along with kmemcheck (when/if that happens)?
>> >
>> > DEFINE_BITFIELD is horrible.
>> >
>> >> @@ -285,11 +286,12 @@ struct sk_buff {
>> >>               };
>> >>       };
>> >>       __u32                   priority;
>> >> -     __u8                    local_df:1,
>> >> +     DEFINE_BITFIELD(__u8,   flags1,
>> >> +                             local_df:1,

[...]

>> Would it help (to make this less
>> horrible) to omit the type declaration and have just the bitfield
>> members as arguments to the macro?
>
> Or you can parse instruction stream a bit more.

I think we have different opinions of what exactly constitutes "horrible" :-P

Your suggestion would have made sense if I was a company of 10
developers who could import all of valgrind source code (including
opcode decoder and instruction emulator) into the kernel. But I only
writing kmemcheck in my free time, so this will never happen. Or.. at
least I will not do it. Of course, kmemcheck, valgrind/memcheck, and
indeed the kernel itself are all open source, so anybody could do it.
And if you want to submit patches yourself, the effort will be welcome
:-)

In the mean-time, I am looking for an acceptable solution. Other
debugging features use helper annotations too. But I am pretty sure
that the immediate solution will not include parsing the instruction
stream a bit more.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] bitfields API

[Vegard Nossum]

On Thu, Aug 28, 2008 at 9:02 PM, Pekka J Enberg <penberg@cs.helsinki.fi> wrote:
> Why not do something like this (as suggested by Ingo, I think)? Yeah, the
> macro should go into kmemcheck.h but I don't have a tree handy...
>
>                Pekka
>
> Index: linux-2.6/include/linux/bitfield.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6/include/linux/bitfield.h
> @@ -0,0 +1,10 @@
> +#ifndef __LINUX_BITFIELD_H
> +#define __LINUX_BITFIELD_H
> +
> +#ifdef CONFIG_KMEMCHECK
> +#define KMEMCHECK_BIT_FIELD(field) do { field = 0; } while (0)
> +#else
> +#define KMEMCHECK_BIT_FIELD(field) do { } while (0)
> +#endif /* CONFIG_KMEMCHECK */
> +
> +#endif /* __LINUX_BITFIELD_H */
>
> Index: linux-2.6/net/core/skbuff.c
> ===================================================================
> --- linux-2.6.orig/net/core/skbuff.c
> +++ linux-2.6/net/core/skbuff.c
> @@ -55,6 +55,7 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/init.h>
>  #include <linux/scatterlist.h>
> +#include <linux/bitfield.h>
>
>  #include <net/protocol.h>
>  #include <net/dst.h>
> @@ -209,6 +210,11 @@ struct sk_buff *__alloc_skb(unsigned int
>        skb->data = data;
>        skb_reset_tail_pointer(skb);
>        skb->end = skb->tail + size;
> +       KMEMCHECK_BIT_FIELD(skb->local_df);
> +       KMEMCHECK_BIT_FIELD(skb->cloned);
> +       KMEMCHECK_BIT_FIELD(skb->ip_summed);
> +       KMEMCHECK_BIT_FIELD(skb->nohdr);
> +       KMEMCHECK_BIT_FIELD(skb->nfctinfo);
>        /* make sure we initialize shinfo sequentially */
>        shinfo = skb_shinfo(skb);
>        atomic_set(&shinfo->dataref, 1);
>

That looks good to me. If the extra lines are okay with net people, we
can put this in the fixlets branch and make it the norm for dealing
with bitfields in kmemcheck.

One thing to keep in mind that if the members of the bitfield does not
span the entire width of the bitfield, the remaining bits must also be
assigned (as an extra "filler" member), otherwise GCC will not
optimize it to a single store. But that is not an issue in this
particular case since all the bits are used.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] bitfields API

[Adrian Bunk]

On Thu, Aug 28, 2008 at 09:40:47PM +0300, Pekka Enberg wrote:
> Hi Alexey,
>
> Alexey Dobriyan wrote:
>> On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
>>> How do you feel about this patch? It's all about making kmemcheck more
>>> useful... and not much else. Does it have any chance of entering the
>>> kernel along with kmemcheck (when/if that happens)?
>>
>> DEFINE_BITFIELD is horrible.
>
> Heh, heh, one alternative is to have a kmemcheck_memset() thingy that  
> unconditionally zeroes bit fields and maybe is a no-op when kmemcheck is  
> disabled.

This sounds as if this might cause bugs to disappear when debugging gets 
turned on?

Or do I miss anything?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC][PATCH] bitfields API

[Pekka Enberg]

Adrian Bunk wrote:
>> Heh, heh, one alternative is to have a kmemcheck_memset() thingy that  
>> unconditionally zeroes bit fields and maybe is a no-op when kmemcheck is  
>> disabled.
> 
> This sounds as if this might cause bugs to disappear when debugging gets 
> turned on?

Yeah, I suppose. The problem doing that unconditionally is that it 
increases kernel text slightly on some architectures (e.g. sparc). 
However, as long as you use the KMEMCHECK_BIT_FIELD annotation only in 
places that give you false positives, it's we should be safe.

		Pekka

Re: [RFC][PATCH] bitfields API

[Vegard Nossum]

On Thu, Aug 28, 2008 at 10:27 PM, Adrian Bunk <bunk@kernel.org> wrote:
> On Thu, Aug 28, 2008 at 09:40:47PM +0300, Pekka Enberg wrote:
>> Hi Alexey,
>>
>> Alexey Dobriyan wrote:
>>> On Thu, Aug 28, 2008 at 08:32:23PM +0200, Vegard Nossum wrote:
>>>> How do you feel about this patch? It's all about making kmemcheck more
>>>> useful... and not much else. Does it have any chance of entering the
>>>> kernel along with kmemcheck (when/if that happens)?
>>>
>>> DEFINE_BITFIELD is horrible.
>>
>> Heh, heh, one alternative is to have a kmemcheck_memset() thingy that
>> unconditionally zeroes bit fields and maybe is a no-op when kmemcheck is
>> disabled.
>
> This sounds as if this might cause bugs to disappear when debugging gets
> turned on?
>
> Or do I miss anything?

You are correct :-)

Almost all the possible solutions (at least the feasible ones) are
trade-offs between false-positives and false-negatives.

So here we are trading a bunch of false-positive errors (a couple of
thousand for transferring a 1M file over ssh :-)) for detecting any
code that uses an uninitialized flag in struct skbuff. So in this case
it is more useful to hide reports about this single bit-field.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [RFC][PATCH] bitfields API

[Vegard Nossum]

On Thu, Aug 28, 2008 at 9:38 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> On Thu, Aug 28, 2008 at 9:02 PM, Pekka J Enberg <penberg@cs.helsinki.fi> wrote:
>> Why not do something like this (as suggested by Ingo, I think)? Yeah, the
>> macro should go into kmemcheck.h but I don't have a tree handy...
>>
>>                Pekka
>>
>> Index: linux-2.6/include/linux/bitfield.h
>> ===================================================================
>> --- /dev/null
>> +++ linux-2.6/include/linux/bitfield.h
>> @@ -0,0 +1,10 @@
>> +#ifndef __LINUX_BITFIELD_H
>> +#define __LINUX_BITFIELD_H
>> +
>> +#ifdef CONFIG_KMEMCHECK
>> +#define KMEMCHECK_BIT_FIELD(field) do { field = 0; } while (0)
>> +#else
>> +#define KMEMCHECK_BIT_FIELD(field) do { } while (0)
>> +#endif /* CONFIG_KMEMCHECK */
>> +
>> +#endif /* __LINUX_BITFIELD_H */
>>
>> Index: linux-2.6/net/core/skbuff.c
>> ===================================================================
>> --- linux-2.6.orig/net/core/skbuff.c
>> +++ linux-2.6/net/core/skbuff.c
>> @@ -55,6 +55,7 @@
>>  #include <linux/rtnetlink.h>
>>  #include <linux/init.h>
>>  #include <linux/scatterlist.h>
>> +#include <linux/bitfield.h>
>>
>>  #include <net/protocol.h>
>>  #include <net/dst.h>
>> @@ -209,6 +210,11 @@ struct sk_buff *__alloc_skb(unsigned int
>>        skb->data = data;
>>        skb_reset_tail_pointer(skb);
>>        skb->end = skb->tail + size;
>> +       KMEMCHECK_BIT_FIELD(skb->local_df);
>> +       KMEMCHECK_BIT_FIELD(skb->cloned);
>> +       KMEMCHECK_BIT_FIELD(skb->ip_summed);
>> +       KMEMCHECK_BIT_FIELD(skb->nohdr);
>> +       KMEMCHECK_BIT_FIELD(skb->nfctinfo);
>>        /* make sure we initialize shinfo sequentially */
>>        shinfo = skb_shinfo(skb);
>>        atomic_set(&shinfo->dataref, 1);
>>
>
> That looks good to me. If the extra lines are okay with net people, we
> can put this in the fixlets branch and make it the norm for dealing
> with bitfields in kmemcheck.
>
> One thing to keep in mind that if the members of the bitfield does not
> span the entire width of the bitfield, the remaining bits must also be
> assigned (as an extra "filler" member), otherwise GCC will not
> optimize it to a single store. But that is not an issue in this
> particular case since all the bits are used.

Hm, and this is exactly the case for the "do_not_encrypt" field of skbuff:

#ifdef CONFIG_IPV6_NDISC_NODETYPE
        __u8                    ndisc_nodetype:2;
#endif
#if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
        __u8                    do_not_encrypt:1;
#endif  

So we have no way to know where or how big the filler should be. This is
why the simple patch above is not sufficient.

Alexey: I have a modified proposal with slightly different syntax for
DEFINE_BITFIELD. Can you say whether this is acceptable or not? Please
see this short mockup example:


<--- cut --->

#include <stdint.h>
#include <string.h>

#define DEFINE_BITFIELD(name, fields...)	\
	union {					\
		struct fields name;		\
		struct fields;			\
	};

#define KMEMCHECK_ANNOTATE_BITFIELD(bitfield)			\
	do {							\
		memset(&(bitfield), 0, sizeof(bitfield));	\
	} while(0)

struct skbuff {
	DEFINE_BITFIELD(flags1, {
		uint8_t			pkt_type:3,
					fclone:2,
					ipvs_property:1,
					peeked:1,
					nf_trace:1;
		uint16_t		protocol;
	});

	DEFINE_BITFIELD(flags2, {
#ifdef CONFIG_IPV6_NDISC_NODETYPE
		uint8_t			ndisc_nodetype:2;
#endif
#if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
		uint8_t			do_not_encrypt:1;
#endif
	});
};

void __alloc_skb(struct skbuff *skb)
{
	KMEMCHECK_ANNOTATE_BITFIELD(skb->flags1);
	KMEMCHECK_ANNOTATE_BITFIELD(skb->flags2);
}

<--- cut --->


Thanks,


Vegard