[PATCH 1/3] libnfnetlink byte alignment

[PATCH 1/3] libnfnetlink byte alignment

[Fabian Hugelshofer]

Aligns buffers to maximum alignment of architecture to make the cast of
char pointers to struct pointers more portable.

Signed-off-by: Fabian Hugelshofer <hugelshofer2006@gmx.ch>


--- libnetfilter_log-0.0.13.orig/src/libnetfilter_log.c	2008-06-02 09:50:47.000000000 +0100
+++ libnetfilter_log-0.0.13/src/libnetfilter_log.c	2008-06-02 18:22:14.000000000 +0100
@@ -107,7 +107,8 @@
 		     u_int16_t queuenum, u_int8_t pf)
 {
 	char buf[NFNL_HEADER_LEN
-		+NFA_LENGTH(sizeof(struct nfulnl_msg_config_cmd))];
+		+NFA_LENGTH(sizeof(struct nfulnl_msg_config_cmd))]
+		__attribute__ ((aligned));
 	struct nfulnl_msg_config_cmd cmd;
 	struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
 
@@ -292,7 +293,8 @@
 		   u_int8_t mode, u_int32_t range)
 {
 	char buf[NFNL_HEADER_LEN
-		+NFA_LENGTH(sizeof(struct nfulnl_msg_config_mode))];
+		+NFA_LENGTH(sizeof(struct nfulnl_msg_config_mode))]
+		__attribute__ ((aligned));
 	struct nfulnl_msg_config_mode params;
 	struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
 
@@ -309,7 +311,8 @@
 
 int nflog_set_timeout(struct nflog_g_handle *gh, u_int32_t timeout)
 {
-	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int32_t))];
+	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int32_t))]
+		__attribute__ ((aligned));
 	struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
 
 	nfnl_fill_hdr(gh->h->nfnlssh, nmh, 0, AF_UNSPEC, gh->id,
@@ -322,7 +325,8 @@
 
 int nflog_set_qthresh(struct nflog_g_handle *gh, u_int32_t qthresh)
 {
-	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int32_t))];
+	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int32_t))]
+		__attribute__ ((aligned));
 	struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
 
 	nfnl_fill_hdr(gh->h->nfnlssh, nmh, 0, AF_UNSPEC, gh->id,
@@ -335,7 +339,8 @@
 
 int nflog_set_nlbufsiz(struct nflog_g_handle *gh, u_int32_t nlbufsiz)
 {
-	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int32_t))];
+	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int32_t))]
+		__attribute__ ((aligned));
 	struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
 	int status;
 
@@ -355,7 +360,8 @@
 
 int nflog_set_flags(struct nflog_g_handle *gh, u_int16_t flags)
 {
-	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int16_t))];
+	char buf[NFNL_HEADER_LEN+NFA_LENGTH(sizeof(u_int16_t))]
+		__attribute__ ((aligned));
 	struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
 
 	nfnl_fill_hdr(gh->h->nfnlssh, nmh, 0, AF_UNSPEC, gh->id,

Re: [PATCH 1/3] libnfnetlink byte alignment

[Patrick McHardy]

Fabian Hugelshofer wrote:
> Aligns buffers to maximum alignment of architecture to make the cast of
> char pointers to struct pointers more portable.
> 
> Signed-off-by: Fabian Hugelshofer <hugelshofer2006@gmx.ch>

They all seem fine to me, but a union might make it look
a bit nicer :)

Re: [PATCH 1/3] libnfnetlink byte alignment

[Fabian Hugelshofer]

Patrick McHardy wrote:
> Fabian Hugelshofer wrote:
>> Aligns buffers to maximum alignment of architecture to make the cast of
>> char pointers to struct pointers more portable.
>>
>> Signed-off-by: Fabian Hugelshofer <hugelshofer2006@gmx.ch>
> 
> They all seem fine to me, but a union might make it look
> a bit nicer :)

In libnfnetlink the union does not make sense, because multiple casts 
are done from the same buffer at different locations.

For libnetfilter-(conntrack|log) the union would be possible:

Original (aligned):
<< code begin >>
char buf[...] __attribute__ ((aligned));
struct nlmsghdr *nmh = (struct nlmsghdr *) buf;

nfnl_fill_hdr(h->nfnlssh, nmh, 0, pf, queuenum, ...);
<< code end >>

Union style:
<< code begin >>
union {
	char buf[...];
	struct nlmsghdr nmh;
} u;

nfnl_fill_hdr(h->nfnlssh, &u.nmh, 0, pf, queuenum, ...);
<< code end >>

I can rewrite it like this, if desired...

Re: [PATCH 1/3] libnfnetlink byte alignment

[Patrick McHardy]

Fabian Hugelshofer wrote:
> Patrick McHardy wrote:
>> Fabian Hugelshofer wrote:
>>> Aligns buffers to maximum alignment of architecture to make the cast of
>>> char pointers to struct pointers more portable.
>>>
>>> Signed-off-by: Fabian Hugelshofer <hugelshofer2006@gmx.ch>
>>
>> They all seem fine to me, but a union might make it look
>> a bit nicer :)
> 
> In libnfnetlink the union does not make sense, because multiple casts 
> are done from the same buffer at different locations.
> 
> For libnetfilter-(conntrack|log) the union would be possible:
> [...]
> 
> I can rewrite it like this, if desired...

I'll leave that up to Pablo, thanks for the explanation.

Re: [PATCH 1/3] libnfnetlink byte alignment

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

Fabian Hugelshofer wrote:
> Patrick McHardy wrote:
>> Fabian Hugelshofer wrote:
>>> Aligns buffers to maximum alignment of architecture to make the cast of
>>> char pointers to struct pointers more portable.
>>>
>>> Signed-off-by: Fabian Hugelshofer <hugelshofer2006@gmx.ch>
>>
>> They all seem fine to me, but a union might make it look
>> a bit nicer :)
> 
> In libnfnetlink the union does not make sense, because multiple casts
> are done from the same buffer at different locations.
> 
> For libnetfilter-(conntrack|log) the union would be possible:
> 
> Original (aligned):
> << code begin >>
> char buf[...] __attribute__ ((aligned));
> struct nlmsghdr *nmh = (struct nlmsghdr *) buf;
> 
> nfnl_fill_hdr(h->nfnlssh, nmh, 0, pf, queuenum, ...);
> << code end >>
> 
> Union style:
> << code begin >>
> union {
>     char buf[...];
>     struct nlmsghdr nmh;
> } u;
> 
> nfnl_fill_hdr(h->nfnlssh, &u.nmh, 0, pf, queuenum, ...);
> << code end >>
> 
> I can rewrite it like this, if desired...

Please, use union wherever it is possible. BTW, it seems to me that the
conntrack-tools may suffer from the same portability problem. The
(supposed) fix, which is attached, is ugly but union does not help since
the size of struct nf_conntrack is not known. I do not find a way to do
this cleanly without removing the use of the stack for temporary object
allocation.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[-- Attachment #2: y --]
[-- Type: text/plain, Size: 2345 bytes --]

diff --git a/src/cache_wt.c b/src/cache_wt.c
index 65a1fc4..9c1b69c 100644
--- a/src/cache_wt.c
+++ b/src/cache_wt.c
@@ -28,7 +28,7 @@
 static void add_wt(struct us_conntrack *u)
 {
 	int ret;
-	char __ct[nfct_maxsize()];
+	char __ct[nfct_maxsize()] __attribute__((aligned));
 	struct nf_conntrack *ct = (struct nf_conntrack *)(void*) __ct;
 
 	ret = nl_exist_conntrack(u->ct);
@@ -56,7 +56,7 @@ static void add_wt(struct us_conntrack *u)
 
 static void upd_wt(struct us_conntrack *u)
 {
-	char __ct[nfct_maxsize()];
+	char __ct[nfct_maxsize()] __attribute__((aligned));
 	struct nf_conntrack *ct = (struct nf_conntrack *)(void*) __ct;
 
 	memcpy(ct, u->ct, nfct_maxsize());
diff --git a/src/conntrack.c b/src/conntrack.c
index 25a3a57..d2e1b47 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -747,7 +747,7 @@ static int update_cb(enum nf_conntrack_msg_type type,
 {
 	int res;
 	struct nf_conntrack *obj = data;
-	char __tmp[nfct_maxsize()];
+	char __tmp[nfct_maxsize()] __attribute__((aligned));
 	struct nf_conntrack *tmp = (struct nf_conntrack *) (void *)__tmp;
 
 	memcpy(tmp, obj, sizeof(__tmp));
@@ -873,14 +873,14 @@ int main(int argc, char *argv[])
 	unsigned int type = 0, event_mask = 0, l4flags = 0, status = 0;
 	int res = 0;
 	int family = AF_UNSPEC;
-	char __obj[nfct_maxsize()];
-	char __exptuple[nfct_maxsize()];
-	char __mask[nfct_maxsize()];
+	char __obj[nfct_maxsize()] __attribute__((aligned));
+	char __exptuple[nfct_maxsize()] __attribute__((aligned));
+	char __mask[nfct_maxsize()] __attribute__((aligned));
 	struct nf_conntrack *obj = (struct nf_conntrack *)(void*) __obj;
 	struct nf_conntrack *exptuple = 
 		(struct nf_conntrack *)(void*) __exptuple;
 	struct nf_conntrack *mask = (struct nf_conntrack *)(void*) __mask;
-	char __exp[nfexp_maxsize()];
+	char __exp[nfexp_maxsize()] __attribute__((aligned));
 	struct nf_expect *exp = (struct nf_expect *)(void*) __exp;
 	int l3protonum;
 	union ct_address ad;
diff --git a/src/sync-mode.c b/src/sync-mode.c
index 4b36935..1b6ec48 100644
--- a/src/sync-mode.c
+++ b/src/sync-mode.c
@@ -38,7 +38,7 @@
 static void do_mcast_handler_step(struct nethdr *net, size_t remain)
 {
 	int query;
-	char __ct[nfct_maxsize()];
+	char __ct[nfct_maxsize()] __attribute__((aligned));
 	struct nf_conntrack *ct = (struct nf_conntrack *)(void*) __ct;
 	struct us_conntrack *u;
 

Re: [PATCH 1/3] libnfnetlink byte alignment

[Fabian Hugelshofer]

Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>> Fabian Hugelshofer wrote:
>>>> Aligns buffers to maximum alignment of architecture to make the cast of
>>>> char pointers to struct pointers more portable.
>>>>
>>>> Signed-off-by: Fabian Hugelshofer <hugelshofer2006@gmx.ch>
>>> They all seem fine to me, but a union might make it look
>>> a bit nicer :)
[...]
> Please, use union wherever it is possible. BTW, it seems to me that the
> conntrack-tools may suffer from the same portability problem. The
> (supposed) fix, which is attached, is ugly but union does not help since
> the size of struct nf_conntrack is not known. I do not find a way to do
> this cleanly without removing the use of the stack for temporary object
> allocation.

As you noted, conntrack-tools can be affected as well. I did not have a 
look at it before, beacause I don't use it.

I don't see another way with the stack either. Dynamic memory allocation 
would help, but is not nicer either.

Do you want me to rewrite the patches for libnetfilter-(log|contrack) in 
the union style? I probably would put the union declaration in the file 
scope rather than anonymous declarations in the function scope, because 
it is the same everywhere.

Re: [PATCH 1/3] libnfnetlink byte alignment

[Pablo Neira Ayuso]

Fabian Hugelshofer wrote:
> Pablo Neira Ayuso wrote:
>> Please, use union wherever it is possible. BTW, it seems to me that the
>> conntrack-tools may suffer from the same portability problem. The
>> (supposed) fix, which is attached, is ugly but union does not help since
>> the size of struct nf_conntrack is not known. I do not find a way to do
>> this cleanly without removing the use of the stack for temporary object
>> allocation.
> 
> As you noted, conntrack-tools can be affected as well. I did not have a
> look at it before, beacause I don't use it.
> 
> I don't see another way with the stack either. Dynamic memory allocation
> would help, but is not nicer either.

I see.

> Do you want me to rewrite the patches for libnetfilter-(log|contrack) in
> the union style? I probably would put the union declaration in the file
> scope rather than anonymous declarations in the function scope, because
> it is the same everywhere.

I'm fine with it. Thanks Fabian.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

Re: [PATCH 1/3] libnfnetlink byte alignment

[Pablo Neira Ayuso]

Fabian Hugelshofer wrote:
> Aligns buffers to maximum alignment of architecture to make the cast of
> char pointers to struct pointers more portable.

Applied. Thanks Fabian.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers