[v2] net: netfilter: LLVMLinux: vlais-netfilter

Message ID 1395123121-27053-1-git-send-email-behanw@converseincode.com
State New
Headers show

Commit Message

Behan Webster March 18, 2014, 6:12 a.m.
From: Mark Charlebois <charlebm@gmail.com>

Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in
xt_repldata.h with a C99 compliant flexible array member and then calculated
offsets to the other struct members. These other members aren't referenced by
name in this code, however this patch maintains the same memory layout and
padding as was previously accomplished using VLAIS.

Had the original structure been ordered differently, with the entries VLA at
the end, then it could have been a flexible member, and this patch would have
been a lot simpler. However since the data stored in this structure is
ultimately exported to userspace, the order of this structure can't be changed.

This patch makes no attempt to change the existing behavior, merely the way in
which the current layout is accomplished using standard C99 constructs. As such
the code can now be compiled with either gcc or clang.

Author: Mark Charlebois <charlebm@gmail.com>
Signed-off-by: Mark Charlebois <charlebm@gmail.com>
Signed-off-by: Behan Webster <behanw@converseincode.com>
Signed-off-by: Vinícius Tinti <viniciustinti@gmail.com>
---
 net/netfilter/xt_repldata.h | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

Comments

David Laight March 18, 2014, 9:41 a.m. | #1
From:  behanw@converseincode.com

> From: Mark Charlebois <charlebm@gmail.com>

> 

> Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in

> xt_repldata.h with a C99 compliant flexible array member and then calculated

> offsets to the other struct members. These other members aren't referenced by

> name in this code, however this patch maintains the same memory layout and

> padding as was previously accomplished using VLAIS.

> 

> Had the original structure been ordered differently, with the entries VLA at

> the end, then it could have been a flexible member, and this patch would have

> been a lot simpler. However since the data stored in this structure is

> ultimately exported to userspace, the order of this structure can't be changed.


Why not just remove the last element and allocate space for it after the
structure?
That would reduce the complexity of the patch and the unreadability
of the new code.
I realise that the alignment of type##_error is 'tricky' to determine.

	David

N‹§˛ćěr¸›yúčšŘb˛XŹśÇ§vŘ^–)Ţş{.nÇ+‰ˇĽŠ{ą‘ęçzX§ś›ĄÜ¨}Šž˛Ć zÚ&j:+v‰¨žŤ‘ęçzZ+€Ę+zfŁ˘ˇhšˆ§~†­†Űi˙űŕzšŽwĽ˘¸?™¨č­Ú&˘)ߢf”ů^jÇŤy§m…á@AŤaśÚ˙0śěhŽĺ’i
Behan Webster March 18, 2014, 2:47 p.m. | #2
On 03/18/14 02:41, David Laight wrote:
> From:  behanw@converseincode.com
>> From: Mark Charlebois <charlebm@gmail.com>
>>
>> Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in
>> xt_repldata.h with a C99 compliant flexible array member and then calculated
>> offsets to the other struct members. These other members aren't referenced by
>> name in this code, however this patch maintains the same memory layout and
>> padding as was previously accomplished using VLAIS.
>>
>> Had the original structure been ordered differently, with the entries VLA at
>> the end, then it could have been a flexible member, and this patch would have
>> been a lot simpler. However since the data stored in this structure is
>> ultimately exported to userspace, the order of this structure can't be changed.
> Why not just remove the last element and allocate space for it after the
> structure?
Because that would still be employing VLAIS to solve the problem. The 
last element may be a zero-length array (a flexible member), not a VLA. 
Sadly both the last 2 elements in the struct need to be manually 
calculated, which is what we've done.

> That would reduce the complexity of the patch and the unreadability
> of the new code.
No one is claiming this patch is more readable, merely that it is C99 
compliant (though strictly speaking this patch is C89, C99 and C11 
compliant). We tried to use macros to make it more readable in previous 
patches. The consensus was that macros were bad.

> I realise that the alignment of type##_error is 'tricky' to determine.
That is what makes it "unreadable". :(

Behan
David Laight March 18, 2014, 3:24 p.m. | #3
From: Behan Webster 

> On 03/18/14 02:41, David Laight wrote:

> > From:  behanw@converseincode.com

> >> From: Mark Charlebois <charlebm@gmail.com>

> >>

> >> Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in

> >> xt_repldata.h with a C99 compliant flexible array member and then calculated

> >> offsets to the other struct members. These other members aren't referenced by

> >> name in this code, however this patch maintains the same memory layout and

> >> padding as was previously accomplished using VLAIS.

> >>

> >> Had the original structure been ordered differently, with the entries VLA at

> >> the end, then it could have been a flexible member, and this patch would have

> >> been a lot simpler. However since the data stored in this structure is

> >> ultimately exported to userspace, the order of this structure can't be changed.

> > Why not just remove the last element and allocate space for it after the

> > structure?

> Because that would still be employing VLAIS to solve the problem. The

> last element may be a zero-length array (a flexible member), not a VLA.

> Sadly both the last 2 elements in the struct need to be manually

> calculated, which is what we've done.


So make the last element a 'flexible member' and then work out where
the final field goes.
Something like:
	struct p {
		struct a a;
		struct b b[];
	} p = malloc(sizeof *p + n * sizeof (struct b) + alignof (struct c)
		+ sizeof (struct c);
	struct c *c = (void *)&p->b[n] + (-offsetof(struct p, b[n]) & (alignof(struct c) - 1);

	David
Behan Webster March 18, 2014, 5:37 p.m. | #4
On 03/18/14 08:24, David Laight wrote:
> From: Behan Webster
>> On 03/18/14 02:41, David Laight wrote:
>>> From:  behanw@converseincode.com
>>>> From: Mark Charlebois <charlebm@gmail.com>
>>>>
>>>> Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in
>>>> xt_repldata.h with a C99 compliant flexible array member and then calculated
>>>> offsets to the other struct members. These other members aren't referenced by
>>>> name in this code, however this patch maintains the same memory layout and
>>>> padding as was previously accomplished using VLAIS.
>>>>
>>>> Had the original structure been ordered differently, with the entries VLA at
>>>> the end, then it could have been a flexible member, and this patch would have
>>>> been a lot simpler. However since the data stored in this structure is
>>>> ultimately exported to userspace, the order of this structure can't be changed.
>>> Why not just remove the last element and allocate space for it after the
>>> structure?
>> Because that would still be employing VLAIS to solve the problem. The
>> last element may be a zero-length array (a flexible member), not a VLA.
>> Sadly both the last 2 elements in the struct need to be manually
>> calculated, which is what we've done.
> So make the last element a 'flexible member' and then work out where
> the final field goes.
> Something like:
> 	struct p {
> 		struct a a;
> 		struct b b[];
> 	} p = malloc(sizeof *p + n * sizeof (struct b) + alignof (struct c)
> 		+ sizeof (struct c);
> 	struct c *c = (void *)&p->b[n] + (-offsetof(struct p, b[n]) & (alignof(struct c) - 1);
Oh, I see. Will fix.

Thanks!

Behan

Patch

diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 6efe4e5..f83a519 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h
@@ -5,29 +5,50 @@ 
  * they serve as the hanging-off data accessed through repl.data[].
  */
 
+/* tbl has the following structure equivalent, but is C99 compliant:
+ * struct {
+ *	struct type##_replace repl;
+ *	struct type##_standard entries[nhooks];
+ *	struct type##_error term;
+ * } *tbl;
+ */
+
 #define xt_alloc_initial_table(type, typ2) ({ \
 	unsigned int hook_mask = info->valid_hooks; \
 	unsigned int nhooks = hweight32(hook_mask); \
 	unsigned int bytes = 0, hooknum = 0, i = 0; \
 	struct { \
 		struct type##_replace repl; \
-		struct type##_standard entries[nhooks]; \
-		struct type##_error term; \
-	} *tbl = kzalloc(sizeof(*tbl), GFP_KERNEL); \
+		char data[0]; \
+	} *tbl; \
+	struct type##_standard *entries; \
+	struct type##_error *term; \
+	size_t entries_offset = (-sizeof(tbl->repl)) & \
+		(__alignof__(*entries)-1); \
+	size_t entries_end = entries_offset + nhooks * sizeof(*entries); \
+	size_t term_offset = entries_end + \
+		(-sizeof(tbl->repl) - entries_end) & (__alignof__(*term)-1); \
+	size_t term_end = term_offset + sizeof(*term); \
+	size_t data_sz = term_end + \
+		(-sizeof(tbl->repl) - term_end) & (__alignof__(tbl->repl)-1); \
+	size_t tbl_sz = sizeof(tbl->repl) + data_sz; \
+	tbl = kzalloc(tbl_sz, GFP_KERNEL); \
 	if (tbl == NULL) \
 		return NULL; \
+	entries = (struct type##_standard *)&tbl->data[entries_offset]; \
+	term = (struct type##_error *)&tbl->data[term_offset]; \
 	strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
-	tbl->term = (struct type##_error)typ2##_ERROR_INIT;  \
+	*term = (struct type##_error)typ2##_ERROR_INIT;  \
 	tbl->repl.valid_hooks = hook_mask; \
 	tbl->repl.num_entries = nhooks + 1; \
 	tbl->repl.size = nhooks * sizeof(struct type##_standard) + \
-	                 sizeof(struct type##_error); \
+			 sizeof(struct type##_error); \
 	for (; hook_mask != 0; hook_mask >>= 1, ++hooknum) { \
 		if (!(hook_mask & 1)) \
 			continue; \
 		tbl->repl.hook_entry[hooknum] = bytes; \
 		tbl->repl.underflow[hooknum]  = bytes; \
-		tbl->entries[i++] = (struct type##_standard) \
+		entries[i++] = (struct type##_standard) \
 			typ2##_STANDARD_INIT(NF_ACCEPT); \
 		bytes += sizeof(struct type##_standard); \
 	} \