Re: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter
On Wed, Mar 19, 2014 at 09:52:40AM +, David Laight wrote:
> From: beh...@converseincode.com
> > From: Mark Charlebois
> >
> > 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
> > Signed-off-by: Mark Charlebois
> > Signed-off-by: Behan Webster
> > Signed-off-by: Vinícius Tinti
> > ---
> > net/netfilter/xt_repldata.h | 27 ++-
> > 1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
> > index 6efe4e5..343599e 100644
> > --- a/net/netfilter/xt_repldata.h
> > +++ b/net/netfilter/xt_repldata.h
> > @@ -5,23 +5,40 @@
> > * 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); \
> > + struct type##_standard entries[]; \
> > + } *tbl; \
> > + struct type##_error *term; \
> > + size_t entries_end = offsetof(typeof(*tbl), \
> > + entries[nhooks-1]) + sizeof(tbl->entries[0]); \
>
> Is the compiler complaining about:
> offsetof(typeof(*tbl), entries[nhooks])
> If it does it is a PITA.
>
> > + size_t term_offset = (entries_end + __alignof__(*term) - 1) \
> > + & ~(__alignof__(*term) - 1); \
>
> You've not tested this - the () are in the wrong places.
I have tested it with both clang and gcc. Unit test is at
http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=test/unit/vlais/netfilter.c;h=7adc255e47e15a252d2bda7af7ae217ac683c25e;hb=HEAD
Basic calulation of a new offset is:
new offset = (offset + align - 1) & ~(align - 1)
The parenthesis seem correct to me.
>
> > + size_t term_end = term_offset + sizeof(*term); \
> > + size_t tbl_sz = (term_end + __alignof__(tbl->repl) - 1) \
> > + & ~(__alignof__(tbl->repl) - 1); \
> > + tbl = kzalloc(tbl_sz, GFP_KERNEL); \
>
> The number of temporary variables make the above hard to read.
> I'm not at all sure you need to worry about the trailing alignment.
> It rather depends on how the final data is used.
> If the combined buffer is copied to userspace you may not
> be copying all of the required data.
> It might be easier to call copytouser() twice.
I can try to remove more variables if that is prefered.
The existing memory layout was preserved exactly so that however the
data currently used will be unaffected. I can remove the trailing
alignment but was not 100% sure it wasn't needed.
>
> > if (tbl == NULL) \
> > return NULL; \
> > + term = (struct type##_error *)&(((char *)tbl)[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; \
>
> David
>
Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter
From: beh...@converseincode.com
> From: Mark Charlebois
>
> 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
> Signed-off-by: Mark Charlebois
> Signed-off-by: Behan Webster
> Signed-off-by: Vinícius Tinti
> ---
> net/netfilter/xt_repldata.h | 27 ++-
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
> index 6efe4e5..343599e 100644
> --- a/net/netfilter/xt_repldata.h
> +++ b/net/netfilter/xt_repldata.h
> @@ -5,23 +5,40 @@
> * 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); \
> + struct type##_standard entries[]; \
> + } *tbl; \
> + struct type##_error *term; \
> + size_t entries_end = offsetof(typeof(*tbl), \
> + entries[nhooks-1]) + sizeof(tbl->entries[0]); \
Is the compiler complaining about:
offsetof(typeof(*tbl), entries[nhooks])
If it does it is a PITA.
> + size_t term_offset = (entries_end + __alignof__(*term) - 1) \
> + & ~(__alignof__(*term) - 1); \
You've not tested this - the () are in the wrong places.
> + size_t term_end = term_offset + sizeof(*term); \
> + size_t tbl_sz = (term_end + __alignof__(tbl->repl) - 1) \
> + & ~(__alignof__(tbl->repl) - 1); \
> + tbl = kzalloc(tbl_sz, GFP_KERNEL); \
The number of temporary variables make the above hard to read.
I'm not at all sure you need to worry about the trailing alignment.
It rather depends on how the final data is used.
If the combined buffer is copied to userspace you may not
be copying all of the required data.
It might be easier to call copytouser() twice.
> if (tbl == NULL) \
> return NULL; \
> + term = (struct type##_error *)&(((char *)tbl)[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; \
David
RE: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter
From: beh...@converseincode.com
From: Mark Charlebois charl...@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 charl...@gmail.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Behan Webster beh...@converseincode.com
Signed-off-by: Vinícius Tinti viniciusti...@gmail.com
---
net/netfilter/xt_repldata.h | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 6efe4e5..343599e 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h
@@ -5,23 +5,40 @@
* 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); \
+ struct type##_standard entries[]; \
+ } *tbl; \
+ struct type##_error *term; \
+ size_t entries_end = offsetof(typeof(*tbl), \
+ entries[nhooks-1]) + sizeof(tbl-entries[0]); \
Is the compiler complaining about:
offsetof(typeof(*tbl), entries[nhooks])
If it does it is a PITA.
+ size_t term_offset = (entries_end + __alignof__(*term) - 1) \
+ ~(__alignof__(*term) - 1); \
You've not tested this - the () are in the wrong places.
+ size_t term_end = term_offset + sizeof(*term); \
+ size_t tbl_sz = (term_end + __alignof__(tbl-repl) - 1) \
+ ~(__alignof__(tbl-repl) - 1); \
+ tbl = kzalloc(tbl_sz, GFP_KERNEL); \
The number of temporary variables make the above hard to read.
I'm not at all sure you need to worry about the trailing alignment.
It rather depends on how the final data is used.
If the combined buffer is copied to userspace you may not
be copying all of the required data.
It might be easier to call copytouser() twice.
if (tbl == NULL) \
return NULL; \
+ term = (struct type##_error *)(((char *)tbl)[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; \
David
Re: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter
On Wed, Mar 19, 2014 at 09:52:40AM +, David Laight wrote:
From: beh...@converseincode.com
From: Mark Charlebois charl...@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 charl...@gmail.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Behan Webster beh...@converseincode.com
Signed-off-by: Vinícius Tinti viniciusti...@gmail.com
---
net/netfilter/xt_repldata.h | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
index 6efe4e5..343599e 100644
--- a/net/netfilter/xt_repldata.h
+++ b/net/netfilter/xt_repldata.h
@@ -5,23 +5,40 @@
* 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); \
+ struct type##_standard entries[]; \
+ } *tbl; \
+ struct type##_error *term; \
+ size_t entries_end = offsetof(typeof(*tbl), \
+ entries[nhooks-1]) + sizeof(tbl-entries[0]); \
Is the compiler complaining about:
offsetof(typeof(*tbl), entries[nhooks])
If it does it is a PITA.
+ size_t term_offset = (entries_end + __alignof__(*term) - 1) \
+~(__alignof__(*term) - 1); \
You've not tested this - the () are in the wrong places.
I have tested it with both clang and gcc. Unit test is at
http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=test/unit/vlais/netfilter.c;h=7adc255e47e15a252d2bda7af7ae217ac683c25e;hb=HEAD
Basic calulation of a new offset is:
new offset = (offset + align - 1) ~(align - 1)
The parenthesis seem correct to me.
+ size_t term_end = term_offset + sizeof(*term); \
+ size_t tbl_sz = (term_end + __alignof__(tbl-repl) - 1) \
+~(__alignof__(tbl-repl) - 1); \
+ tbl = kzalloc(tbl_sz, GFP_KERNEL); \
The number of temporary variables make the above hard to read.
I'm not at all sure you need to worry about the trailing alignment.
It rather depends on how the final data is used.
If the combined buffer is copied to userspace you may not
be copying all of the required data.
It might be easier to call copytouser() twice.
I can try to remove more variables if that is prefered.
The existing memory layout was preserved exactly so that however the
data currently used will be unaffected. I can remove the trailing
alignment but was not 100% sure it wasn't needed.
if (tbl == NULL) \
return NULL; \
+ term = (struct type##_error *)(((char *)tbl)[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; \
David
Mark
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
