ipt_physdev.c alignment problems on parisc64

ipt_physdev.c alignment problems on parisc64

[Arnaldo Carvalho de Melo]

The 1.786.1.54 changeset (i.e. the initial ipt_physdev.c one 8) created this
alignment problems on parisc64:

tmp.stdout.allegro.31506.s: Assembler messages:
tmp.stdout.allegro.31506.s:97: Error: Field not properly aligned [8] (18).
tmp.stdout.allegro.31506.s:97: Error: Invalid operands
tmp.stdout.allegro.31506.s:125: Error: Field not properly aligned [8] (34).
tmp.stdout.allegro.31506.s:125: Error: Invalid operands
tmp.stdout.allegro.31506.s:129: Error: Field not properly aligned [8] (50).
tmp.stdout.allegro.31506.s:129: Error: Invalid operands

I got this nailed to the match function, on these lines:

        for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
                ret |= (((const unsigned long *)indev)[i]
                        ^ ((const unsigned long *)info->physindev)[i])
                        & ((const unsigned long *)info->in_mask)[i];
        }

and

        for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
                ret |= (((const unsigned long *)outdev)[i]
                        ^ ((const unsigned long *)info->physoutdev)[i])
                        & ((const unsigned long *)info->out_mask)[i];
        }


Reading specs from /opt/palinux/lib/gcc-lib/hppa64-linux/3.0.4/specs
Configured with: ../../gcc-3.0-3.0.4ds3/src/configure --host=hppa-linux --build=hppa-linux --target=hppa64-linux --disable-shared --disable-nls --enable-languages=c --prefix=/opt/palinux
Thread model: single
gcc version 3.0.4
acme@allegro:~/bk/scsi-2.6$

- Arnaldo

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Tuesday 02 September 2003 16:30, Arnaldo Carvalho de Melo wrote:
> The 1.786.1.54 changeset (i.e. the initial ipt_physdev.c one 8) created

Does this fix it?

cheers,
Bart

--- linux-2.6.0-test4/include/linux/netfilter_ipv4/ipt_physdev.h.old	2003-09-02 21:11:12.000000000 +0200
+++ linux-2.6.0-test4/include/linux/netfilter_ipv4/ipt_physdev.h	2003-09-02 21:12:30.000000000 +0200
@@ -15,7 +15,7 @@
 struct ipt_physdev_info {
 	u_int8_t invert;
 	u_int8_t bitmask;
-	char physindev[IFNAMSIZ];
+	char physindev[IFNAMSIZ] __attribute__ ((aligned (__alignof__(unsigned long))));
 	char in_mask[IFNAMSIZ];
 	char physoutdev[IFNAMSIZ];
 	char out_mask[IFNAMSIZ];

Re: ipt_physdev.c alignment problems on parisc64

[Arnaldo Carvalho de Melo]

Em Tue, Sep 02, 2003 at 09:16:41PM +0200, Bart De Schuymer escreveu:
> On Tuesday 02 September 2003 16:30, Arnaldo Carvalho de Melo wrote:
> > The 1.786.1.54 changeset (i.e. the initial ipt_physdev.c one 8) created
> 
> Does this fix it?

Thanks, I'll test it later, now I have most of my machines turned off while
cleaning up this messy machine room.

- Arnaldo

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Tue, 2 Sep 2003 21:16:41 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> On Tuesday 02 September 2003 16:30, Arnaldo Carvalho de Melo wrote:
> > The 1.786.1.54 changeset (i.e. the initial ipt_physdev.c one 8) created
> 
> Does this fix it?

You can't use this fix.  This header and structure are used by
userspace and "unsigned long" can be a different size in the
kernel than it is in user space.

Please, just remove the super-silly memcmp() optimization in
the ipt_physdev.c code.

Re: ipt_physdev.c alignment problems on parisc64

[Harald Welte]

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

On Wed, Sep 03, 2003 at 08:04:26PM -0700, David S. Miller wrote:
> On Tue, 2 Sep 2003 21:16:41 +0200
> Bart De Schuymer <bdschuym@pandora.be> wrote:
> 
> > On Tuesday 02 September 2003 16:30, Arnaldo Carvalho de Melo wrote:
> > > The 1.786.1.54 changeset (i.e. the initial ipt_physdev.c one 8) created
> > 
> > Does this fix it?
> 
> You can't use this fix.  This header and structure are used by
> userspace and "unsigned long" can be a different size in the
> kernel than it is in user space.
> 
> Please, just remove the super-silly memcmp() optimization in
> the ipt_physdev.c code.

Dave, the respective code in ipt_physdev.c seems to be copied from
the ip_tables.c interface name match (which definitely has the same
alignment issues, btw).

The problem is that it is _not_ a simple reimplementation of memcmp(),
but a mask-compare.

People can do stuff like "-i ppp+", meaning that traffic from all
interfaces called "ppp<WHATEVER>" are matched.

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Fri, 5 Sep 2003 17:31:52 +0200
Harald Welte <laforge@netfilter.org> wrote:

> The problem is that it is _not_ a simple reimplementation of memcmp(),
> but a mask-compare.

Ok.  It still should be fixed by not using the "unsigned long *"
cast-and-op tricks...

Re: ipt_physdev.c alignment problems on parisc64

[jamal]

Harald,

Could you not resolve all the ifindices of the said ppp+ interfaces
at rule installation time and do an integer compare instead?

cheers,
jamal

On Fri, 2003-09-05 at 11:31, Harald Welte wrote:

> > 
> > Please, just remove the super-silly memcmp() optimization in
> > the ipt_physdev.c code.
> 
> Dave, the respective code in ipt_physdev.c seems to be copied from
> the ip_tables.c interface name match (which definitely has the same
> alignment issues, btw).
> 
> The problem is that it is _not_ a simple reimplementation of memcmp(),
> but a mask-compare.
> 
> People can do stuff like "-i ppp+", meaning that traffic from all
> interfaces called "ppp<WHATEVER>" are matched.

Re: ipt_physdev.c alignment problems on parisc64

[Harald Welte]

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

On Thu, Sep 11, 2003 at 09:40:46PM -0400, jamal wrote:
> Harald,
> 
> Could you not resolve all the ifindices of the said ppp+ interfaces
> at rule installation time and do an integer compare instead?

No, this is a different semantic.  Ifindexes change when an interface
goes down and comes up again (let's say you have a PPTP tunnel server
which terminates a couple of ppp* interfaces).

What could be done, is register with the netdev notifiers and then at
every ifup/ifdown event change the ifindex[es] in all rules that use 
this style of match.

This is what was done in the 2.2 ipchains code, if I am not mistaken.
However, Rusty went for the string-compare solution while implementing
iptables.  

Imagine somebody with lots of ppp interfaces (let's say 40)... and we
would need to resolve all 40 into ifindexes, put them into an array...
how big would you like to make the array?  how many interfaces do people
have? how much space do you want to waste (dynamically reallocating this
space while the ruleset is already loaded is not possible).

And then you have 1000 rules, each of it using a ppp* style match. than
at every ifdown/ifup you iterate over 1000 rules, checking if you need
to update one of the 40 ifindexes?  quite slow...

> cheers,
> jamal
-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_physdev.c alignment problems on parisc64

[Oskar Andreasson]

On Fri, 12 Sep 2003, Harald Welte wrote:

<snip>
>
> Imagine somebody with lots of ppp interfaces (let's say 40)... and we
> would need to resolve all 40 into ifindexes, put them into an array...
> how big would you like to make the array?  how many interfaces do people
> have? how much space do you want to waste (dynamically reallocating this
> space while the ruleset is already loaded is not possible).
>

Just to add my point o this, and to say that it is actually used in
real-world. We have several (>30) machines with 120 modems each, each
running a pppd (modem pool, yes), and some  300-400 iptables rules on each
machine.

> And then you have 1000 rules, each of it using a ppp* style match. than
> at every ifdown/ifup you iterate over 1000 rules, checking if you need
> to update one of the 40 ifindexes?  quite slow...
>
> > cheers,
> > jamal
> - Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/

----
Oskar Andreasson
http://www.frozentux.net
http://iptables-tutorial.frozentux.net
http://ipsysctl-tutorial.frozentux.net
mailto:blueflux@koffein.net

Re: ipt_physdev.c alignment problems on parisc64

[jamal]

On Fri, 2003-09-12 at 04:56, Harald Welte wrote:
> On Thu, Sep 11, 2003 at 09:40:46PM -0400, jamal wrote:
> > Harald,
> > 
> > Could you not resolve all the ifindices of the said ppp+ interfaces
> > at rule installation time and do an integer compare instead?
> 
> No, this is a different semantic.  Ifindexes change when an interface
> goes down and comes up again (let's say you have a PPTP tunnel server
> which terminates a couple of ppp* interfaces).
> 

If ifindices change for the same device name then that would be a bug.
The only time they should change is if a) you reboot or b) you unload a
module after ifconfiged down the device(s) and then reload it later.
In both cases if you have exactly the same setup, the chances of those
ifindices changing are very slim.

> What could be done, is register with the netdev notifiers and then at
> every ifup/ifdown event change the ifindex[es] in all rules that use 
> this style of match.
> 

You may wanna listen to new devices being created and add to the ifindex
list. 

> This is what was done in the 2.2 ipchains code, if I am not mistaken.
> However, Rusty went for the string-compare solution while implementing
> iptables.  
> 
> Imagine somebody with lots of ppp interfaces (let's say 40)... and we
> would need to resolve all 40 into ifindexes, put them into an array...
> how big would you like to make the array?  how many interfaces do people
> have? how much space do you want to waste (dynamically reallocating this
> space while the ruleset is already loaded is not possible).
> 

This is a valid reason for the string compare given the way iptables is
architected.

> And then you have 1000 rules, each of it using a ppp* style match. than
> at every ifdown/ifup you iterate over 1000 rules, checking if you need
> to update one of the 40 ifindexes?  quite slow...

you could optimize. There can be only one ppp* list for all rules to
share and you iterate not the rules rather the table which holds the
ifindices. Could be 1-2 lookup to find the ifindex if proper hashing is
used.

cheers,
jamal

Re: ipt_physdev.c alignment problems on parisc64

[jamal]

On Fri, 2003-09-12 at 06:56, Oskar Andreasson wrote:

> Just to add my point o this, and to say that it is actually used in
> real-world. We have several (>30) machines with 120 modems each, each
> running a pppd (modem pool, yes), and some  300-400 iptables rules on each
> machine.
> 

How it is presented to the user - and how it is actually implemented are
two different things.
you could still enter it as ppp+ 

cheers,
jamal

Re: ipt_physdev.c alignment problems on parisc64

[Oskar Andreasson]

On Fri, 12 Sep 2003, jamal wrote:

>
> How it is presented to the user - and how it is actually implemented are
> two different things.
> you could still enter it as ppp+
>

Of course I am aware of that, I just wanted to push my view on it since I
believe there must be many more than us doing the same thing (or more
modems per machine for that matter). However, I was a little bit worried
that someone might be pushing a solution that may be much slower in our
way of looking at it ;).

> cheers,
> jamal
>

----
Oskar Andreasson
http://www.frozentux.net
http://iptables-tutorial.frozentux.net
http://ipsysctl-tutorial.frozentux.net
mailto:blueflux@koffein.net

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Tuesday 09 September 2003 04:14, David S. Miller wrote:
> On Fri, 5 Sep 2003 17:31:52 +0200
>
> Harald Welte <laforge@netfilter.org> wrote:
> > The problem is that it is _not_ a simple reimplementation of memcmp(),
> > but a mask-compare.
>
> Ok.  It still should be fixed by not using the "unsigned long *"
> cast-and-op tricks...

The patch below should fix things. It just uses unsigned int instead
of unsigned long, so there should be no problem with the evil Sparc64.
Perhaps the ip_tables.c code from which I copied the original code should
be altered too.
It's a real optimization btw, the .o file is 32 bytes
larger when doing the bit operations on a char with i<IFNAMSIZ.
All hail Rusty :)
The patch doesn't change the .o file on my i386 system. It will probably
be slower for systems where sizeof(unsigned long)>sizeof(unsigned int),
but this is the easiest solution for me.

cheers,
Bart

--- linux-2.6.0-test5/net/ipv4/netfilter/ipt_physdev.c.old	2003-09-11 21:04:12.000000000 +0200
+++ linux-2.6.0-test5/net/ipv4/netfilter/ipt_physdev.c	2003-09-13 21:42:23.000000000 +0200
@@ -23,7 +23,7 @@ match(const struct sk_buff *skb,
 	int i;
 	static const char nulldevname[IFNAMSIZ];
 	const struct ipt_physdev_info *info = matchinfo;
-	unsigned long ret;
+	unsigned int ret;
 	const char *indev, *outdev;
 	struct nf_bridge_info *nf_bridge;
 
@@ -65,10 +65,10 @@ match(const struct sk_buff *skb,
 	if (!(info->bitmask & IPT_PHYSDEV_OP_IN))
 		goto match_outdev;
 	indev = nf_bridge->physindev ? nf_bridge->physindev->name : nulldevname;
-	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)indev)[i]
-			^ ((const unsigned long *)info->physindev)[i])
-			& ((const unsigned long *)info->in_mask)[i];
+	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned int); i++) {
+		ret |= (((const unsigned int *)indev)[i]
+			^ ((const unsigned int *)info->physindev)[i])
+			& ((const unsigned int *)info->in_mask)[i];
 	}
 
 	if ((ret == 0) ^ !(info->invert & IPT_PHYSDEV_OP_IN))
@@ -79,10 +79,10 @@ match_outdev:
 		return MATCH;
 	outdev = nf_bridge->physoutdev ?
 		 nf_bridge->physoutdev->name : nulldevname;
-	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)outdev)[i]
-			^ ((const unsigned long *)info->physoutdev)[i])
-			& ((const unsigned long *)info->out_mask)[i];
+	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned int); i++) {
+		ret |= (((const unsigned int *)outdev)[i]
+			^ ((const unsigned int *)info->physoutdev)[i])
+			& ((const unsigned int *)info->out_mask)[i];
 	}
 
 	return (ret != 0) ^ !(info->invert & IPT_PHYSDEV_OP_OUT);
--- linux-2.6.0-test5/include/linux/netfilter_ipv4/ipt_physdev.h.old	2003-09-13 21:32:23.000000000 +0200
+++ linux-2.6.0-test5/include/linux/netfilter_ipv4/ipt_physdev.h	2003-09-13 21:33:04.000000000 +0200
@@ -15,7 +15,7 @@
 struct ipt_physdev_info {
 	u_int8_t invert;
 	u_int8_t bitmask;
-	char physindev[IFNAMSIZ];
+	char physindev[IFNAMSIZ] __attribute__ ((aligned (__alignof__(unsigned int))));
 	char in_mask[IFNAMSIZ];
 	char physoutdev[IFNAMSIZ];
 	char out_mask[IFNAMSIZ];

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Sat, 13 Sep 2003 21:59:37 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> It just uses unsigned int instead of unsigned long, so there should
> be no problem with the evil Sparc64.

Sparc64 doesn't have the problems, it just fixes up the unaligned
accesses there so it'll just be super-slow (some "optimization" :).

The problem is on parisc64 which is very strict about accessing
structure members which are not aligned to the accessor type you use
to dereference and gives you a link/compile error when you violate
this.

> Perhaps the ip_tables.c code from which I copied the original code
> should be altered too.

Yes.

> It's a real optimization btw, the .o file is 32 bytes
> larger when doing the bit operations on a char with i<IFNAMSIZ.
> All hail Rusty :)

I understand, but instead create an arch routine to optimize this
instead of using non-portable constructs.  memcmp_masked() or
something like that.

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Sat, 13 Sep 2003 21:59:37 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> -	char physindev[IFNAMSIZ];
> +	char physindev[IFNAMSIZ] __attribute__ ((aligned (__alignof__(unsigned int))));

I have to reject the fix for this problem again.

You can't change the struct exported to userspace in any way
without breaking the tools.  I tried to explain this in the
first revision of your change.

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Tue, 16 Sep 2003 08:05:07 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> Anyway, this rule about not changing structs exported to userspace truly 
> sucks, IMHO.

Welcome to the real world.

> If someone decides to compile her own kernel this person should 
> not expect every userspace tool to keep working, again IMHO.

Wouldn't it be great if you recompiled you kernel and the args
to the read() system call got rearranged?

There is simply no difference in this case.  The situation doesn't
change because the object is being passed into some netfilter module.

You can't arbitrarily change userland exported APIs at your convenience
to fix some bug.  You either have to create a new interface for the user
to use, or fix the bug without changing the API.

Will it be the end of the world if you do a byte at a time comparison,
at least as a temporary solution?

Also, another solution could be to store the object inside the kernel
using a different structure, one where you can guarentee the alignment
of these things properly.

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Tuesday 16 September 2003 00:59, David S. Miller wrote:
> > -	char physindev[IFNAMSIZ];
> > +	char physindev[IFNAMSIZ] __attribute__ ((aligned (__alignof__(unsigned
> > int))));
>
> I have to reject the fix for this problem again.
>
> You can't change the struct exported to userspace in any way
> without breaking the tools.  I tried to explain this in the
> first revision of your change.

On Thursday 04 September 2003 05:05, you wrote:
<quote>
As I mentioned in another email, this patch can't be used, it
breaks in mixed 32/64 bit environments.
<unquote>

I guess I'm a bit on the slow side but I didn't quite read the above text like 
that.
Anyway, this rule about not changing structs exported to userspace truly 
sucks, IMHO. If someone decides to compile her own kernel this person should 
not expect every userspace tool to keep working, again IMHO. Adding a new 
module to the kernel results in the user having to upgrade her tools, 
recompiling won't even do. Why on earth do you allow adding modules.
But I'm sure some nice volunteer will come along and downgrade that code 
fragment.

Bart

Re: ipt_physdev.c alignment problems on parisc64

[Henrik Nordstrom]

tis 2003-09-16 klockan 08.02 skrev David S. Miller:

> Also, another solution could be to store the object inside the kernel
> using a different structure, one where you can guarentee the alignment
> of these things properly.

unfortunately iptables does not allow such differences between userspace
and kernel representation..

-- 
Henrik Nordstrom <hno@marasystems.com>
MARA Systems AB

Re: ipt_physdev.c alignment problems on parisc64

[Harald Welte]

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

On Mon, Sep 15, 2003 at 03:59:03PM -0700, David S. Miller wrote:

> I have to reject the fix for this problem again.
> 
> You can't change the struct exported to userspace in any way
> without breaking the tools.  I tried to explain this in the
> first revision of your change.

But ipt_physdev is a 2.6ism, thus we can still introduce any
incompatibility... For me it's enough to be compatible within 2.4 or
within 2.6.x (once 2.6.0 is out).

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_physdev.c alignment problems on parisc64

[Harald Welte]

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

On Tue, Sep 16, 2003 at 11:18:34AM +0200, Henrik Nordstrom wrote:
> tis 2003-09-16 klockan 08.02 skrev David S. Miller:
> 
> > Also, another solution could be to store the object inside the kernel
> > using a different structure, one where you can guarentee the alignment
> > of these things properly.
> 
> unfortunately iptables does not allow such differences between userspace
> and kernel representation..

yes. yes. yes.  The whole 'kernel internal data structure exported to
userspace' is a fundamental design flaw of iptables.  There's nothing we
can do about it, we'll have to live with it :(

That's why the yet-to-be-completed pkttables will use what I call vTLV's
(versioned TLV's) for all [netlink based] kernel-userspace
communication.

> Henrik Nordstrom <hno@marasystems.com>
> MARA Systems AB

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_physdev.c alignment problems on parisc64

[Tom Marshall]

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

On Tue, Sep 16, 2003 at 04:06:21PM +0200, Harald Welte wrote:
> On Mon, Sep 15, 2003 at 03:59:03PM -0700, David S. Miller wrote:
> 
> > I have to reject the fix for this problem again.
> > 
> > You can't change the struct exported to userspace in any way
> > without breaking the tools.  I tried to explain this in the
> > first revision of your change.
> 
> But ipt_physdev is a 2.6ism, thus we can still introduce any
> incompatibility... For me it's enough to be compatible within 2.4 or
> within 2.6.x (once 2.6.0 is out).

Noooo!  Please don't break compatibility for 2.6.  I really don't want to
have separate /sbin/iptables-2.4 and /sbin/iptables-2.6 binaries.

-- 
Logic is a systematic method of coming to the wrong conclusion with
confidence.

[-- Attachment #2: Type: application/pgp-signature, Size: 240 bytes --]

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Tuesday 16 September 2003 17:15, Tom Marshall wrote:
> > But ipt_physdev is a 2.6ism, thus we can still introduce any
> > incompatibility... For me it's enough to be compatible within 2.4 or
> > within 2.6.x (once 2.6.0 is out).
>
> Noooo!  Please don't break compatibility for 2.6.  I really don't want to
> have separate /sbin/iptables-2.4 and /sbin/iptables-2.6 binaries.

2.4 hasn't got ipt_physdev

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Tue, 16 Sep 2003 16:09:18 +0200
Harald Welte <laforge@netfilter.org> wrote:

> On Tue, Sep 16, 2003 at 11:18:34AM +0200, Henrik Nordstrom wrote:
> > tis 2003-09-16 klockan 08.02 skrev David S. Miller:
> > 
> > unfortunately iptables does not allow such differences between userspace
> > and kernel representation..
> 
> yes. yes. yes.  The whole 'kernel internal data structure exported to
> userspace' is a fundamental design flaw of iptables.  There's nothing we
> can do about it, we'll have to live with it :(

Why can't you change this?

Just because the user gives the kernel the data in one format, this
does not at all prevent the actual iptables kernel implementation from
using some other structure to store the data.  You just have to translate
things on the way in and out, that's all.

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Tue, 16 Sep 2003 17:57:50 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> 2.4 hasn't got ipt_physdev

But recall that there are other netfilter modules doing this
"compare chars using cast to long" trick that need to be fixed
too.

Also, adding attributes to user exported structures would need
to be done _very_ carefully, as compilation when using such
gcc extensions will fail when the user uses '-ansi' on the
compiler command line.

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Wednesday 17 September 2003 03:23, David S. Miller wrote:
> But recall that there are other netfilter modules doing this
> "compare chars using cast to long" trick that need to be fixed
> too.

Well, acme hasn't reported any other issues. Perhaps Rusty had foresight about 
these possible issues and made sure alignment was good...
In which case I don't see any reason for altering his code.

> Also, adding attributes to user exported structures would need
> to be done _very_ carefully, as compilation when using such
> gcc extensions will fail when the user uses '-ansi' on the
> compiler command line.

Now why on Earth would I have to bother with some luser doing this? I'm not 
the slave of annoying users.

cheers,
Bart

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Wed, 17 Sep 2003 08:42:17 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> > Also, adding attributes to user exported structures would need
> > to be done _very_ carefully, as compilation when using such
> > gcc extensions will fail when the user uses '-ansi' on the
> > compiler command line.
> 
> Now why on Earth would I have to bother with some luser doing this? I'm not 
> the slave of annoying users.

Because you can make his life simpler merely by adding the right
__extension__() around the attribute designation, just like glibc
does in it's headers.  It takes nearly no effort on your part and
makes life simpler for everyone writing userspace programs.

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Wednesday 17 September 2003 03:16, David S. Miller wrote:
> > yes. yes. yes.  The whole 'kernel internal data structure exported to
> > userspace' is a fundamental design flaw of iptables.  There's nothing we
> > can do about it, we'll have to live with it :(
>
> Why can't you change this?
>
> Just because the user gives the kernel the data in one format, this
> does not at all prevent the actual iptables kernel implementation from
> using some other structure to store the data.  You just have to translate
> things on the way in and out, that's all.

In theory this is ofcourse possible. In practice this would mean changing
ip_tables.c, which is not a good idea.

Below is a patch that does a char-by-char compare for ipt_physdev.c.
Since there have not been any reports about these alignment problems for
other similar code, I'd wait with changing any of them.
It's still not too late to apply the other patch that just aligned the
struct member instead.
Let me try to convince you one more time :)
- as Harald stated it's 2.6 only, so these userspace incompatibilities
are no problem
- it is too much trouble to get right alignment without messing up
userspace compatibility (ip_tables.c needs to be changed)
- even if the other similar device comparing code needs to be fixed, there
is no reason why ipt_physdev should be slowed down like all the rest.
- the code below is slower

It's up to you, I'm not religious about this stuff.

cheers,
Bart


--- linux-2.6.0-test5/net/ipv4/netfilter/ipt_physdev.c.old	2003-09-17 22:55:53.000000000 +0200
+++ linux-2.6.0-test5/net/ipv4/netfilter/ipt_physdev.c	2003-09-17 22:58:17.000000000 +0200
@@ -23,7 +23,7 @@ match(const struct sk_buff *skb,
 	int i;
 	static const char nulldevname[IFNAMSIZ];
 	const struct ipt_physdev_info *info = matchinfo;
-	unsigned long ret;
+	unsigned int ret;
 	const char *indev, *outdev;
 	struct nf_bridge_info *nf_bridge;
 
@@ -65,11 +65,8 @@ match(const struct sk_buff *skb,
 	if (!(info->bitmask & IPT_PHYSDEV_OP_IN))
 		goto match_outdev;
 	indev = nf_bridge->physindev ? nf_bridge->physindev->name : nulldevname;
-	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)indev)[i]
-			^ ((const unsigned long *)info->physindev)[i])
-			& ((const unsigned long *)info->in_mask)[i];
-	}
+	for (i = 0, ret = 0; i < IFNAMSIZ; i++)
+		ret |= (indev[i] ^ info->physindev[i]) & info->in_mask[i];
 
 	if ((ret == 0) ^ !(info->invert & IPT_PHYSDEV_OP_IN))
 		return NOMATCH;
@@ -79,11 +76,8 @@ match_outdev:
 		return MATCH;
 	outdev = nf_bridge->physoutdev ?
 		 nf_bridge->physoutdev->name : nulldevname;
-	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)outdev)[i]
-			^ ((const unsigned long *)info->physoutdev)[i])
-			& ((const unsigned long *)info->out_mask)[i];
-	}
+	for (i = 0, ret = 0; i < IFNAMSIZ; i++)
+		ret |= (outdev[i] ^ info->physoutdev[i]) & info->out_mask[i];
 
 	return (ret != 0) ^ !(info->invert & IPT_PHYSDEV_OP_OUT);
 }

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Wed, 17 Sep 2003 23:17:38 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> Let me try to convince you one more time :)

Sure, but let me give you a suggestion, OK?

Don't use an alignment attribute, just put the member of
the struct at the very beginning and use "unsigned int"
at a time op+comparisons.

How about that?

Re: ipt_physdev.c alignment problems on parisc64

[Harald Welte]

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

On Fri, Sep 12, 2003 at 08:54:29AM -0400, jamal wrote:
> On Fri, 2003-09-12 at 04:56, Harald Welte wrote:
> > On Thu, Sep 11, 2003 at 09:40:46PM -0400, jamal wrote:
> > > Harald,
> > > 
> > > Could you not resolve all the ifindices of the said ppp+ interfaces
> > > at rule installation time and do an integer compare instead?
> > 
> > No, this is a different semantic.  Ifindexes change when an interface
> > goes down and comes up again (let's say you have a PPTP tunnel server
> > which terminates a couple of ppp* interfaces).
> > 
> 
> If ifindices change for the same device name then that would be a bug.
> The only time they should change is if a) you reboot or b) you unload a
> module after ifconfiged down the device(s) and then reload it later.
> In both cases if you have exactly the same setup, the chances of those
> ifindices changing are very slim.

What about a ppp interface that is deleted because pppd terminated.
Then you start a new pppd that uses the same ppp interface [i.e. ppp12].
Would ifindex still be guaranteed to be the same in that case?

> cheers,
> jamal

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_physdev.c alignment problems on parisc64

[Bart De Schuymer]

On Friday 19 September 2003 04:48, David S. Miller wrote:
> Don't use an alignment attribute, just put the member of
> the struct at the very beginning and use "unsigned int"
> at a time op+comparisons.
>
> How about that?

Fine with me.

cheers,
Bart


--- linux-2.6.0-test5/include/linux/netfilter_ipv4/ipt_physdev.h.old	2003-09-20 13:28:26.000000000 +0200
+++ linux-2.6.0-test5/include/linux/netfilter_ipv4/ipt_physdev.h	2003-09-20 13:29:06.000000000 +0200
@@ -13,12 +13,12 @@
 #define IPT_PHYSDEV_OP_MASK		(0x20 - 1)
 
 struct ipt_physdev_info {
-	u_int8_t invert;
-	u_int8_t bitmask;
 	char physindev[IFNAMSIZ];
 	char in_mask[IFNAMSIZ];
 	char physoutdev[IFNAMSIZ];
 	char out_mask[IFNAMSIZ];
+	u_int8_t invert;
+	u_int8_t bitmask;
 };
 
 #endif /*_IPT_PHYSDEV_H*/
--- linux-2.6.0-test5/net/ipv4/netfilter/ipt_physdev.c.old	2003-09-20 13:28:44.000000000 +0200
+++ linux-2.6.0-test5/net/ipv4/netfilter/ipt_physdev.c	2003-09-20 13:29:06.000000000 +0200
@@ -23,7 +23,7 @@ match(const struct sk_buff *skb,
 	int i;
 	static const char nulldevname[IFNAMSIZ];
 	const struct ipt_physdev_info *info = matchinfo;
-	unsigned long ret;
+	unsigned int ret;
 	const char *indev, *outdev;
 	struct nf_bridge_info *nf_bridge;
 
@@ -65,10 +65,10 @@ match(const struct sk_buff *skb,
 	if (!(info->bitmask & IPT_PHYSDEV_OP_IN))
 		goto match_outdev;
 	indev = nf_bridge->physindev ? nf_bridge->physindev->name : nulldevname;
-	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)indev)[i]
-			^ ((const unsigned long *)info->physindev)[i])
-			& ((const unsigned long *)info->in_mask)[i];
+	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned int); i++) {
+		ret |= (((const unsigned int *)indev)[i]
+			^ ((const unsigned int *)info->physindev)[i])
+			& ((const unsigned int *)info->in_mask)[i];
 	}
 
 	if ((ret == 0) ^ !(info->invert & IPT_PHYSDEV_OP_IN))
@@ -79,10 +79,10 @@ match_outdev:
 		return MATCH;
 	outdev = nf_bridge->physoutdev ?
 		 nf_bridge->physoutdev->name : nulldevname;
-	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
-		ret |= (((const unsigned long *)outdev)[i]
-			^ ((const unsigned long *)info->physoutdev)[i])
-			& ((const unsigned long *)info->out_mask)[i];
+	for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned int); i++) {
+		ret |= (((const unsigned int *)outdev)[i]
+			^ ((const unsigned int *)info->physoutdev)[i])
+			& ((const unsigned int *)info->out_mask)[i];
 	}
 
 	return (ret != 0) ^ !(info->invert & IPT_PHYSDEV_OP_OUT);

Re: ipt_physdev.c alignment problems on parisc64

[Arnaldo Carvalho de Melo]

Em Sat, Sep 20, 2003 at 01:32:12PM +0200, Bart De Schuymer escreveu:
> On Friday 19 September 2003 04:48, David S. Miller wrote:
> > Don't use an alignment attribute, just put the member of
> > the struct at the very beginning and use "unsigned int"
> > at a time op+comparisons.
> >
> > How about that?
> 
> Fine with me.

Fixes the problem on parisc64, just tested, thanks.

- Arnaldo

ppp ifindex WAS(Re: ipt_physdev.c alignment problems on parisc64

[jamal]

On Sat, 2003-09-20 at 01:50, Harald Welte wrote:

> > 
> > If ifindices change for the same device name then that would be a bug.
> > The only time they should change is if a) you reboot or b) you unload a
> > module after ifconfiged down the device(s) and then reload it later.
> > In both cases if you have exactly the same setup, the chances of those
> > ifindices changing are very slim.
> 
> What about a ppp interface that is deleted because pppd terminated.
> Then you start a new pppd that uses the same ppp interface [i.e. ppp12].
> Would ifindex still be guaranteed to be the same in that case?
> 

maybe not in that case - but i am not sure if it is a feature or a bug; 
The reason is because pppd will create a brand new device everytime but
may maintain the same name as you point out. I suppose this is legit.
The whole point behind the ifindex is to do device management, billing,
etc. Example if you were doing fault detection/failover and something
goes wrong with ppp12, you can uniqueuly identify it is the same ppp12
that you are interested in if the ifindex is the same. Chances are no if
you depend on the name as you do in iptables. Infact the new ppp12 maybe
terminating at a new server. For this reason, maybe a good idea for ppp
_not_ to reuse the same unit number but rather to monotomically
increment so that next ppp device you get is ppp13. Yes, I know it will
break a lot of scripts etc. Unless you dont care about the above
scenario.
OTOH, take eth* - in this case it is desirable to maintain the same
ifindex even across reboots for consistency as above. Note, tc doesnt
have these issues.

cheers,
jamal

Re: ipt_physdev.c alignment problems on parisc64

[David S. Miller]

On Sat, 20 Sep 2003 13:32:12 +0200
Bart De Schuymer <bdschuym@pandora.be> wrote:

> On Friday 19 September 2003 04:48, David S. Miller wrote:
> > Don't use an alignment attribute, just put the member of
> > the struct at the very beginning and use "unsigned int"
> > at a time op+comparisons.
> >
> > How about that?
> 
> Fine with me.

Patch applied, thanks Bart.