On Thu, Aug 28, 2008 at 9:38 PM, Vegard Nossum  wrote:

quoted text
> On Thu, Aug 28, 2008 at 9:02 PM, Pekka J Enberg  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

>>  #include

>>  #include

>> +#include

>>

>>  #include

>>  #include

>> @@ -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

#include 
#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

--

unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

the body of a message to majordomo@vger.kernel.org

More majordomo info at  http://vger.kernel.org/majordomo-info.html

Please read the FAQ at  http://www.tux.org/lkml/