Re: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter

[Mark Charlebois <charlebm@gmail.com>]

On Wed, Mar 19, 2014 at 09:52:40AM +0000, 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.
> > 
> > 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 | 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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html