Re: different packing of structs in kernel vs. userland ?
On Sun, 2002/07/14 at 23:08:21 -0400, Mike Barcroft wrote:
Thomas Moestl [EMAIL PROTECTED] writes:
(Disclaimer: my solution below is untested, so it may all be bogus)
As request, here are the test results.
Most rules work, except my final one:
%%%
bowie# ipfw add allow all from any to any
ipfw: getsockopt(IP_FW_ADD): Invalid argument
%%%
Oh, right, that's related: the kernel checks for a minimum size of the
passed data on two occasions, first in sooptcopyin(), and then again
in check_ipfw_struct().
It the size to be at least sizeof(struct ip_fw), however for
structures containing just one action (like the one for the command
above) this is again too much in the 64-bit case because of the
padding. Can you please try the attached patch (against the CVS
version)?
- thomas
--
Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/
[EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/
PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C
Index: ip_fw.h
===
RCS file: /home/ncvs/src/sys/netinet/ip_fw.h,v
retrieving revision 1.71
diff -u -r1.71 ip_fw.h
--- ip_fw.h 8 Jul 2002 22:39:19 - 1.71
+++ ip_fw.h 15 Jul 2002 10:48:19 -
@@ -294,8 +294,9 @@
#define ACTION_PTR(rule) \
(ipfw_insn *)( (u_int32_t *)((rule)-cmd) + ((rule)-act_ofs) )
-#define RULESIZE(rule) (sizeof(struct ip_fw) + \
- ((struct ip_fw *)(rule))-cmd_len * 4 - 4)
+#defineRULESIZE_FROMLEN(len) (offsetof(struct ip_fw, cmd) + (len) * 4)
+#defineRULESIZE(rule) RULESIZE_FROMLEN(((struct ip_fw *)(rule))-cmd_len)
+#defineRULESIZE_MINRULESIZE_FROMLEN(1)
/*
* This structure is used as a flow mask and a flow id for various
Index: ip_fw2.c
===
RCS file: /home/ncvs/src/sys/netinet/ip_fw2.c,v
retrieving revision 1.4
diff -u -r1.4 ip_fw2.c
--- ip_fw2.c8 Jul 2002 22:46:01 - 1.4
+++ ip_fw2.c15 Jul 2002 10:38:09 -
@@ -2142,7 +2142,7 @@
int have_action=0;
ipfw_insn *cmd;
- if (size sizeof(*rule)) {
+ if (size RULESIZE_MIN) {
printf(ipfw: rule too short\n);
return (EINVAL);
}
@@ -2428,7 +2428,7 @@
case IP_FW_ADD:
rule = (struct ip_fw *)rule_buf; /* XXX do a malloc */
error = sooptcopyin(sopt, rule, sizeof(rule_buf),
- sizeof(struct ip_fw) );
+ RULESIZE_MIN);
size = sopt-sopt_valsize;
if (error || (error = check_ipfw_struct(rule, size)))
break;
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
sorry but all this just does not make sense to me.
sizeof(foo) should give the same result irrespective of
where you use it.
Perhaps the best thing would be to put a
printf(struct ip_fw has size %d\n, sizeof(struct ip_fw));
both in ipfw2.c and somewhere in ip_fw2.c and see if there is
a mismatch between the two numbers.
cheers
luigi
On Mon, Jul 15, 2002 at 12:51:58PM +0200, Thomas Moestl wrote:
On Sun, 2002/07/14 at 23:08:21 -0400, Mike Barcroft wrote:
Thomas Moestl [EMAIL PROTECTED] writes:
(Disclaimer: my solution below is untested, so it may all be bogus)
As request, here are the test results.
Most rules work, except my final one:
%%%
bowie# ipfw add allow all from any to any
ipfw: getsockopt(IP_FW_ADD): Invalid argument
%%%
Oh, right, that's related: the kernel checks for a minimum size of the
passed data on two occasions, first in sooptcopyin(), and then again
in check_ipfw_struct().
It the size to be at least sizeof(struct ip_fw), however for
structures containing just one action (like the one for the command
above) this is again too much in the 64-bit case because of the
padding. Can you please try the attached patch (against the CVS
version)?
- thomas
--
Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/
[EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/
PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C
Index: ip_fw.h
===
RCS file: /home/ncvs/src/sys/netinet/ip_fw.h,v
retrieving revision 1.71
diff -u -r1.71 ip_fw.h
--- ip_fw.h 8 Jul 2002 22:39:19 - 1.71
+++ ip_fw.h 15 Jul 2002 10:48:19 -
@@ -294,8 +294,9 @@
#define ACTION_PTR(rule) \
(ipfw_insn *)( (u_int32_t *)((rule)-cmd) + ((rule)-act_ofs) )
-#define RULESIZE(rule) (sizeof(struct ip_fw) + \
- ((struct ip_fw *)(rule))-cmd_len * 4 - 4)
+#define RULESIZE_FROMLEN(len) (offsetof(struct ip_fw, cmd) + (len) * 4)
+#define RULESIZE(rule) RULESIZE_FROMLEN(((struct ip_fw *)(rule))-cmd_len)
+#define RULESIZE_MINRULESIZE_FROMLEN(1)
/*
* This structure is used as a flow mask and a flow id for various
Index: ip_fw2.c
===
RCS file: /home/ncvs/src/sys/netinet/ip_fw2.c,v
retrieving revision 1.4
diff -u -r1.4 ip_fw2.c
--- ip_fw2.c 8 Jul 2002 22:46:01 - 1.4
+++ ip_fw2.c 15 Jul 2002 10:38:09 -
@@ -2142,7 +2142,7 @@
int have_action=0;
ipfw_insn *cmd;
- if (size sizeof(*rule)) {
+ if (size RULESIZE_MIN) {
printf(ipfw: rule too short\n);
return (EINVAL);
}
@@ -2428,7 +2428,7 @@
case IP_FW_ADD:
rule = (struct ip_fw *)rule_buf; /* XXX do a malloc */
error = sooptcopyin(sopt, rule, sizeof(rule_buf),
- sizeof(struct ip_fw) );
+ RULESIZE_MIN);
size = sopt-sopt_valsize;
if (error || (error = check_ipfw_struct(rule, size)))
break;
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
On Mon, 2002/07/15 at 04:00:08 -0700, Luigi Rizzo wrote: sorry but all this just does not make sense to me. sizeof(foo) should give the same result irrespective of where you use it. OK, let me rephrase it: as I explained before, struct ip_fw has padding after 'cmd' (the last member) to ensure that arrays can be built from it safely, so that the first member will always be properly aligned. Since the first members must/should be aligned on an 8-bit boundary on 64-bit platforms, this means that sizeof(struct ip_fw) must be a multiple of 8, the size of the padding is 4 bytes (unless the situation is changed by reordering structure members). This can easily be checked on a 64-bit platform. The following program fragment: struct ip_fw f; printf(sizeof(ip_fw) = %d\n, (int)sizeof(f)); printf(offsetof(ip_fw, cmd) = %d\n, (int)offsetof(struct ip_fw, cmd)); printf(sizeof(ip_fw.cmd) = %d\n, (int)sizeof(f.cmd)); Produces this output on sparc64: sizeof(ip_fw) = 56 offsetof(ip_fw, cmd) = 48 sizeof(ip_fw.cmd) = 4 This illustrates that indeed, padding is appended after 'cmd'. In the (userland) ipfw2.c, you basically do the following: ipfw_insn *dst; /* sizeof(ipfw_insn) = 4 */ dst = (ipfw_insn *)rule-cmd; /* Write n instructions and increase dst accordingly. */ rule-cmd_len = (u_int32_t *)dst - (u_int32_t *)(rule-cmd); i = (void *)dst - (void *)rule; if (getsockopt(s, IPPROTO_IP, IP_FW_ADD, rule, i) == -1) err(EX_UNAVAILABLE, getsockopt(%s), IP_FW_ADD); Let's consider the case where only one instruction was added. In this case, dst was incremented once and points directly after cmd, so i is 52 on a 64-bit platform. However, sizeof(struct ip_fw) is 56 because the aformentioned 4 bytes of padding following 'cmd', so i sizeof(struct ip_fw). This explains why rules with just one instruction would not work properly in this case with just my first patch. Likewise, when adding more rules, the second one will be added to the memory location directly following 'cmd'. If padding is present, the second instruction will write into it. The size of the total structure will thus not be properly computed by the old RULESIZE macro: #define RULESIZE(rule) (sizeof(struct ip_fw) + \ ((struct ip_fw *)(rule))-cmd_len * 4 - 4) The '- 4' is meant to subtract the size of the cmd, which is accounted for in cmd_len. Still, you are counting the padding twice, once in the sizeof() and once in cmd_len. So, sizeof(struct ip_fw) is no different between userland and kernel, but the problem is that you don't use sizeof(struct ip_fw) in userland to compute the sizes (but pointer arithmetic), but you do use it for the checks in the kernel. - thomas -- Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/ [EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/ PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
On Mon, 2002/07/15 at 04:24:33 -0700, Terry Lambert wrote: Luigi Rizzo wrote: sorry but all this just does not make sense to me. sizeof(foo) should give the same result irrespective of where you use it. Perhaps the best thing would be to put a printf(struct ip_fw has size %d\n, sizeof(struct ip_fw)); both in ipfw2.c and somewhere in ip_fw2.c and see if there is a mismatch between the two numbers. I have to assume that what didn't make sense was that his patch worked? 8-). He's making the valid point that for: struct foo *fee; It's possible that: sizeof(struct foo) != (((char *)fee[1]) - ((char *)fee[0])) No, I do not. In fact, the opposite: sizeof(struct foo) = (((char *)fee[1]) - ((char *)fee[0])) _must_ always be true, since it is legal to compute the size of storage needed for an n-element array of struct foo by using (sizeof(struct foo) * n). My point was that, because of the above, any padding that might be required between the first and last member of two struct foo's immediately following each other must be _included_ in struct foo, after the last element. - thomas -- Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/ [EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/ PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
Thomas Moestl [EMAIL PROTECTED] writes: Oh, right, that's related: the kernel checks for a minimum size of the passed data on two occasions, first in sooptcopyin(), and then again in check_ipfw_struct(). It the size to be at least sizeof(struct ip_fw), however for structures containing just one action (like the one for the command above) this is again too much in the 64-bit case because of the padding. Can you please try the attached patch (against the CVS version)? Yes, this version works. %%% bowie# ipfw show 00100 0 0 allow ip from me to 192.168.3.1 00200 5484 allow udp from me to 192.168.3.13 00300 0 0 allow tcp from me to 192.168.3.0/24 established 00400 0 0 deny ip from me to 192.168.3.0/24 00500 9734 allow ip from any to any 65535 0 0 deny ip from any to any %%% Best regards, Mike Barcroft To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
Richard Tobin wrote: Er, no, that's not right. Otherwise [ ... ] If everyone could read the text past my example of bad math, so that they could know it was an intentional example of bad math, live would be beautiful. 8-). -- Terry To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
On Tue, 16 Jul 2002, David Taylor wrote: Bah, ignore me, it appears you've already admitted your post was rather less than clear :) -- David Taylor [EMAIL PROTECTED] The future just ain't what it used to be To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
Luigi Rizzo wrote:
Hi,
the following message seems to suggest that the compiler
(the way it is invoked) packs structures differently
when building the kernel and userland.
The stize of the structure in question is computed
by both kernel and userland app using sizeof(),
so there is no assumption on the size of its members,
so i believe the only possibility of a mismatch is
the one above.
Any ideas ?
Probably a #pragma pack() in scope.
Add one explicitly before and after the structure definition
to force it to be the same in user space and the kernel, e.g.:
#pragma pack(1) /* 1 byte packing */
struct foo {
...
};
#pragma pack() /* whatever it was before */
The latter is like a pop of the former's push, so you
should be OK, even though it looks like what is happening
is an include of an include file in the wrong place or in
code that has one in scope.
-- Terry
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
On Sun, 2002/07/14 at 01:18:10 -0700, Luigi Rizzo wrote:
Hi,
the following message seems to suggest that the compiler
(the way it is invoked) packs structures differently
when building the kernel and userland.
The stize of the structure in question is computed
by both kernel and userland app using sizeof(),
so there is no assumption on the size of its members,
so i believe the only possibility of a mismatch is
the one above.
Any ideas ?
(Disclaimer: my solution below is untested, so it may all be bogus)
No, you are not accounting for external structure padding. Take a
look:
struct ip_fw {
struct ip_fw *next; /* linked list of rules */
u_int16_t act_ofs;/* offset of action in 32-bit units */
u_int16_t cmd_len;/* # of 32-bit words in cmd */
u_int16_t rulenum;/* rule number */
u_int16_t _pad; /* padding */
/* These fields are present in all rules. */
u_int64_t pcnt; /* Packet counter */
u_int64_t bcnt; /* Byte counter */
u_int32_t timestamp; /* tv_sec of last match */
struct ip_fw *next_rule; /* ptr to next rule */
ipfw_insn cmd[1]; /* storage for commands */
};
On a 64-bit architecture, pointers are obviously 8 bytes in size;
structure members must or should be on natural borders, depending on
the architecture.
So, next_rule will not be on a natural border; 4 bytes of padding will
be inserted before it. With that, the total structure size would be
52.
The compiler must account for the fact that an array of struct ip_fws
may be used. For obvious reasons, it can not just insert extra padding
in the array case; instead, the structure size must be chosen so that
in this situation, the first member will be on a natural border.
This results in an extra 4 bytes of external padding at the end,
after the member 'cmd'.
The macro you use to compute the size in the kernel is:
#define RULESIZE(rule) (sizeof(struct ip_fw) + \
((struct ip_fw *)(rule))-cmd_len * 4 - 4)
In the userland code, you start at foo.cmd and append data
directly. This means that the padding will also be used to store
data, so the '- 4' (= sizeof(foo.cmd)) will not always be enough. The
following definition of RULESIZE (untested) should fix this:
#define RULESIZE(rule) (offsetof(struct ip_fw, cmd) + \
((struct ip_fw *)(rule))-cmd_len * 4)
It also removes the explicit 4 for sizeof(ipfw_insn).
- thomas
--
Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/
[EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/
PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
[i am deliberately not trimming the email in case someone wants to
look at the context]
i am a bit dubious about your explaination -- it also does not
explain why the person reporting this problem fixed that
by swapping timestamp and next_rule in the structure
cheers
luigi
On Sun, Jul 14, 2002 at 10:36:42PM +0200, Thomas Moestl wrote:
On Sun, 2002/07/14 at 01:18:10 -0700, Luigi Rizzo wrote:
Hi,
the following message seems to suggest that the compiler
(the way it is invoked) packs structures differently
when building the kernel and userland.
The stize of the structure in question is computed
by both kernel and userland app using sizeof(),
so there is no assumption on the size of its members,
so i believe the only possibility of a mismatch is
the one above.
Any ideas ?
(Disclaimer: my solution below is untested, so it may all be bogus)
No, you are not accounting for external structure padding. Take a
look:
struct ip_fw {
struct ip_fw *next; /* linked list of rules */
u_int16_t act_ofs;/* offset of action in 32-bit units */
u_int16_t cmd_len;/* # of 32-bit words in cmd */
u_int16_t rulenum;/* rule number */
u_int16_t _pad; /* padding */
/* These fields are present in all rules. */
u_int64_t pcnt; /* Packet counter */
u_int64_t bcnt; /* Byte counter */
u_int32_t timestamp; /* tv_sec of last match */
struct ip_fw *next_rule; /* ptr to next rule */
ipfw_insn cmd[1]; /* storage for commands */
};
On a 64-bit architecture, pointers are obviously 8 bytes in size;
structure members must or should be on natural borders, depending on
the architecture.
So, next_rule will not be on a natural border; 4 bytes of padding will
be inserted before it. With that, the total structure size would be
52.
The compiler must account for the fact that an array of struct ip_fws
may be used. For obvious reasons, it can not just insert extra padding
in the array case; instead, the structure size must be chosen so that
in this situation, the first member will be on a natural border.
This results in an extra 4 bytes of external padding at the end,
after the member 'cmd'.
The macro you use to compute the size in the kernel is:
#define RULESIZE(rule) (sizeof(struct ip_fw) + \
((struct ip_fw *)(rule))-cmd_len * 4 - 4)
In the userland code, you start at foo.cmd and append data
directly. This means that the padding will also be used to store
data, so the '- 4' (= sizeof(foo.cmd)) will not always be enough. The
following definition of RULESIZE (untested) should fix this:
#define RULESIZE(rule) (offsetof(struct ip_fw, cmd) + \
((struct ip_fw *)(rule))-cmd_len * 4)
It also removes the explicit 4 for sizeof(ipfw_insn).
- thomas
--
Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/
[EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/
PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: different packing of structs in kernel vs. userland ?
On Sun, 2002/07/14 at 13:43:37 -0700, Luigi Rizzo wrote: [i am deliberately not trimming the email in case someone wants to look at the context] i am a bit dubious about your explaination -- it also does not explain why the person reporting this problem fixed that by swapping timestamp and next_rule in the structure It does - doing so removes the need for padding before 'next_rule', because it is properly aligned then. 'timestamp' and 'cmd' are both 4 bytes in size and immediately follow each other, so the total structure size is a multiple of 8 (48 bytes). Because of that, no padding after 'cmd' is required, and the effect is gone. - thomas -- Thomas Moestl [EMAIL PROTECTED] http://www.tu-bs.de/~y0015675/ [EMAIL PROTECTED] http://people.FreeBSD.org/~tmm/ PGP fingerprint: 1C97 A604 2BD0 E492 51D0 9C0F 1FE6 4F1D 419C 776C To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
