[PATCH] kmemcheck: make bitfield annotations truly no-ops when disabled

[PATCH] kmemcheck: make bitfield annotations truly no-ops when disabled

[Vegard Nossum]

It turns out that even zero-sized struct members (int foo[0];) will affect
the struct layout, causing us in particular to lose 4 bytes in struct sock.

This patch fixes the regression in CONFIG_KMEMCHECK=n case.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 include/linux/kmemcheck.h |  110 ++++++++++++++++++++++++---------------------
 1 files changed, 58 insertions(+), 52 deletions(-)

Andrew: Compile tested on x86_32 and x86_64, but because of my bad track
record when it comes to brown paper bugs, can you please keep this in -mm
for a little while? Thanks.

diff --git a/include/linux/kmemcheck.h b/include/linux/kmemcheck.h
index e880d4c..08d7dc4 100644
--- a/include/linux/kmemcheck.h
+++ b/include/linux/kmemcheck.h
@@ -36,6 +36,56 @@ int kmemcheck_hide_addr(unsigned long address);
 
 bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size);
 
+/*
+ * Bitfield annotations
+ *
+ * How to use: If you have a struct using bitfields, for example
+ *
+ *     struct a {
+ *             int x:8, y:8;
+ *     };
+ *
+ * then this should be rewritten as
+ *
+ *     struct a {
+ *             kmemcheck_bitfield_begin(flags);
+ *             int x:8, y:8;
+ *             kmemcheck_bitfield_end(flags);
+ *     };
+ *
+ * Now the "flags_begin" and "flags_end" members may be used to refer to the
+ * beginning and end, respectively, of the bitfield (and things like
+ * &x.flags_begin is allowed). As soon as the struct is allocated, the bit-
+ * fields should be annotated:
+ *
+ *     struct a *a = kmalloc(sizeof(struct a), GFP_KERNEL);
+ *     kmemcheck_annotate_bitfield(a, flags);
+ */
+#define kmemcheck_bitfield_begin(name)	\
+	int name##_begin[0];
+
+#define kmemcheck_bitfield_end(name)	\
+	int name##_end[0];
+
+#define kmemcheck_annotate_bitfield(ptr, name)				\
+	do {								\
+		int _n;							\
+									\
+		if (!ptr)						\
+			break;						\
+									\
+		_n = (long) &((ptr)->name##_end)			\
+			- (long) &((ptr)->name##_begin);		\
+		MAYBE_BUILD_BUG_ON(_n < 0);				\
+									\
+		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
+	} while (0)
+
+#define kmemcheck_annotate_variable(var)				\
+	do {								\
+		kmemcheck_mark_initialized(&(var), sizeof(var));	\
+	} while (0)							\
+
 #else
 #define kmemcheck_enabled 0
 
@@ -106,60 +156,16 @@ static inline bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size)
 	return true;
 }
 
-#endif /* CONFIG_KMEMCHECK */
-
-/*
- * Bitfield annotations
- *
- * How to use: If you have a struct using bitfields, for example
- *
- *     struct a {
- *             int x:8, y:8;
- *     };
- *
- * then this should be rewritten as
- *
- *     struct a {
- *             kmemcheck_bitfield_begin(flags);
- *             int x:8, y:8;
- *             kmemcheck_bitfield_end(flags);
- *     };
- *
- * Now the "flags_begin" and "flags_end" members may be used to refer to the
- * beginning and end, respectively, of the bitfield (and things like
- * &x.flags_begin is allowed). As soon as the struct is allocated, the bit-
- * fields should be annotated:
- *
- *     struct a *a = kmalloc(sizeof(struct a), GFP_KERNEL);
- *     kmemcheck_annotate_bitfield(a, flags);
- *
- * Note: We provide the same definitions for both kmemcheck and non-
- * kmemcheck kernels. This makes it harder to introduce accidental errors. It
- * is also allowed to pass NULL pointers to kmemcheck_annotate_bitfield().
- */
-#define kmemcheck_bitfield_begin(name)	\
-	int name##_begin[0];
-
-#define kmemcheck_bitfield_end(name)	\
-	int name##_end[0];
+#define kmemcheck_bitfield_begin(name)
+#define kmemcheck_bitfield_end(name)
+#define kmemcheck_annotate_bitfield(ptr, name)	\
+	do {					\
+	} while (0)
 
-#define kmemcheck_annotate_bitfield(ptr, name)				\
-	do {								\
-		int _n;							\
-									\
-		if (!ptr)						\
-			break;						\
-									\
-		_n = (long) &((ptr)->name##_end)			\
-			- (long) &((ptr)->name##_begin);		\
-		MAYBE_BUILD_BUG_ON(_n < 0);				\
-									\
-		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
+#define kmemcheck_annotate_variable(var)	\
+	do {					\
 	} while (0)
 
-#define kmemcheck_annotate_variable(var)				\
-	do {								\
-		kmemcheck_mark_initialized(&(var), sizeof(var));	\
-	} while (0)							\
+#endif /* CONFIG_KMEMCHECK */
 
 #endif /* LINUX_KMEMCHECK_H */
-- 
1.6.0.6

Re: [PATCH] kmemcheck: make bitfield annotations truly no-ops when disabled

[Pekka Enberg]

Vegard Nossum kirjoitti:
> It turns out that even zero-sized struct members (int foo[0];) will affect
> the struct layout, causing us in particular to lose 4 bytes in struct sock.
> 
> This patch fixes the regression in CONFIG_KMEMCHECK=n case.
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ---
>  include/linux/kmemcheck.h |  110 ++++++++++++++++++++++++---------------------
>  1 files changed, 58 insertions(+), 52 deletions(-)
> 
> Andrew: Compile tested on x86_32 and x86_64, but because of my bad track
> record when it comes to brown paper bugs, can you please keep this in -mm
> for a little while? Thanks.
> 
> diff --git a/include/linux/kmemcheck.h b/include/linux/kmemcheck.h
> index e880d4c..08d7dc4 100644
> --- a/include/linux/kmemcheck.h
> +++ b/include/linux/kmemcheck.h
> @@ -36,6 +36,56 @@ int kmemcheck_hide_addr(unsigned long address);
>  
>  bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size);
>  
> +/*
> + * Bitfield annotations
> + *
> + * How to use: If you have a struct using bitfields, for example
> + *
> + *     struct a {
> + *             int x:8, y:8;
> + *     };
> + *
> + * then this should be rewritten as
> + *
> + *     struct a {
> + *             kmemcheck_bitfield_begin(flags);
> + *             int x:8, y:8;
> + *             kmemcheck_bitfield_end(flags);
> + *     };
> + *
> + * Now the "flags_begin" and "flags_end" members may be used to refer to the
> + * beginning and end, respectively, of the bitfield (and things like
> + * &x.flags_begin is allowed). As soon as the struct is allocated, the bit-
> + * fields should be annotated:
> + *
> + *     struct a *a = kmalloc(sizeof(struct a), GFP_KERNEL);
> + *     kmemcheck_annotate_bitfield(a, flags);
> + */
> +#define kmemcheck_bitfield_begin(name)	\
> +	int name##_begin[0];
> +
> +#define kmemcheck_bitfield_end(name)	\
> +	int name##_end[0];
> +
> +#define kmemcheck_annotate_bitfield(ptr, name)				\
> +	do {								\
> +		int _n;							\
> +									\
> +		if (!ptr)						\
> +			break;						\
> +									\
> +		_n = (long) &((ptr)->name##_end)			\
> +			- (long) &((ptr)->name##_begin);		\
> +		MAYBE_BUILD_BUG_ON(_n < 0);				\
> +									\
> +		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
> +	} while (0)
> +
> +#define kmemcheck_annotate_variable(var)				\
> +	do {								\
> +		kmemcheck_mark_initialized(&(var), sizeof(var));	\
> +	} while (0)							\
> +
>  #else
>  #define kmemcheck_enabled 0
>  
> @@ -106,60 +156,16 @@ static inline bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size)
>  	return true;
>  }
>  
> -#endif /* CONFIG_KMEMCHECK */
> -
> -/*
> - * Bitfield annotations
> - *
> - * How to use: If you have a struct using bitfields, for example
> - *
> - *     struct a {
> - *             int x:8, y:8;
> - *     };
> - *
> - * then this should be rewritten as
> - *
> - *     struct a {
> - *             kmemcheck_bitfield_begin(flags);
> - *             int x:8, y:8;
> - *             kmemcheck_bitfield_end(flags);
> - *     };
> - *
> - * Now the "flags_begin" and "flags_end" members may be used to refer to the
> - * beginning and end, respectively, of the bitfield (and things like
> - * &x.flags_begin is allowed). As soon as the struct is allocated, the bit-
> - * fields should be annotated:
> - *
> - *     struct a *a = kmalloc(sizeof(struct a), GFP_KERNEL);
> - *     kmemcheck_annotate_bitfield(a, flags);
> - *
> - * Note: We provide the same definitions for both kmemcheck and non-
> - * kmemcheck kernels. This makes it harder to introduce accidental errors. It
> - * is also allowed to pass NULL pointers to kmemcheck_annotate_bitfield().
> - */
> -#define kmemcheck_bitfield_begin(name)	\
> -	int name##_begin[0];
> -
> -#define kmemcheck_bitfield_end(name)	\
> -	int name##_end[0];
> +#define kmemcheck_bitfield_begin(name)
> +#define kmemcheck_bitfield_end(name)
> +#define kmemcheck_annotate_bitfield(ptr, name)	\
> +	do {					\
> +	} while (0)
>  
> -#define kmemcheck_annotate_bitfield(ptr, name)				\
> -	do {								\
> -		int _n;							\
> -									\
> -		if (!ptr)						\
> -			break;						\
> -									\
> -		_n = (long) &((ptr)->name##_end)			\
> -			- (long) &((ptr)->name##_begin);		\
> -		MAYBE_BUILD_BUG_ON(_n < 0);				\
> -									\
> -		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
> +#define kmemcheck_annotate_variable(var)	\
> +	do {					\
>  	} while (0)
>  
> -#define kmemcheck_annotate_variable(var)				\
> -	do {								\
> -		kmemcheck_mark_initialized(&(var), sizeof(var));	\
> -	} while (0)							\
> +#endif /* CONFIG_KMEMCHECK */
>  
>  #endif /* LINUX_KMEMCHECK_H */

Re: [PATCH] kmemcheck: make bitfield annotations truly no-ops when disabled

[Andrew Morton]

On Sun,  6 Dec 2009 18:42:13 +0100
Vegard Nossum <vegard.nossum@gmail.com> wrote:

> It turns out that even zero-sized struct members (int foo[0];) will affect
> the struct layout, causing us in particular to lose 4 bytes in struct sock.
> 
> This patch fixes the regression in CONFIG_KMEMCHECK=n case.

Which kernel version is the regression relative to?

Should we backport this into 2.6.32.x?

If so, we have a slight problem: someone did
s/MAYBE_BUILD_BUG_ON/BUILD_BUG_ON/ in linux-next (I have feeling it was
me ;)) so the patch I applied had that slight massaging.  Perhaps the
-stable stalwarts can take care of that slight difference.

Re: [PATCH] kmemcheck: make bitfield annotations truly no-ops when disabled

[Pekka Enberg]

Andrew Morton kirjoitti:
> On Sun,  6 Dec 2009 18:42:13 +0100
> Vegard Nossum <vegard.nossum@gmail.com> wrote:
> 
>> It turns out that even zero-sized struct members (int foo[0];) will affect
>> the struct layout, causing us in particular to lose 4 bytes in struct sock.
>>
>> This patch fixes the regression in CONFIG_KMEMCHECK=n case.
> 
> Which kernel version is the regression relative to?

AFAICT, it dates back to 2.6.31 when kmemcheck was introduced in 
mainline. Vegard?

> Should we backport this into 2.6.32.x?

I guess so. Eric, how bad is the regression?

			Pekka

Re: [PATCH] kmemcheck: make bitfield annotations truly no-ops when disabled

[Eric Dumazet]

Le 09/12/2009 14:31, Pekka Enberg a écrit :

> 
> I guess so. Eric, how bad is the regression?
> 

Not too bad, skbuff one was separately addressed.

http://git2.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=14d18a81b5171d4433e41129619c75748b4f4d26