The following code casts pointers to char to long in order to do a fast
comparison. This causes alignment errors on IA64 and likely also on
other platforms:
/* Look for ifname matches; this should unroll nicely. */
for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
ret |= (((const unsigned long *)indev)[i]
^ ((const unsigned long *)arpinfo->iniface)[i])
& ((const unsigned long *)arpinfo->iniface_mask)[i];
}
iniface is a pointer to char.
A patch for this was sent to the netfilter-devel list a while back,
see this link:
http://marc.theaimsgroup.com/?l=netfilter-devel&m=107814727803971&w=2
I don't think the patch ever made it anywhere so far.
Alex
On Wed, 2004-06-09 at 12:09, Christoph Lameter wrote:
> The following code casts pointers to char to long in order to do a fast
> comparison. This causes alignment errors on IA64 and likely also on
> other platforms:
>
> /* Look for ifname matches; this should unroll nicely. */
> for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
> ret |= (((const unsigned long *)indev)[i]
> ^ ((const unsigned long *)arpinfo->iniface)[i])
> & ((const unsigned long *)arpinfo->iniface_mask)[i];
> }
>
> iniface is a pointer to char.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Wed, 09 Jun 2004 12:27:56 -0600
Alex Williamson <[email protected]> wrote:
> http://marc.theaimsgroup.com/?l=netfilter-devel&m=107814727803971&w=2
How can you legitimately change this structure? It's an exported
userland interface, if you change it all the applications will
stop working.
On Wed, 2004-06-09 at 14:00, David S. Miller wrote:
> On Wed, 09 Jun 2004 12:27:56 -0600
> Alex Williamson <[email protected]> wrote:
>
> > http://marc.theaimsgroup.com/?l=netfilter-devel&m=107814727803971&w=2
>
> How can you legitimately change this structure? It's an exported
> userland interface, if you change it all the applications will
> stop working.
>
Which is probably why the patch never went anywhere. There's
certainly an alignment issue in the usage of the struct arpt_arp in the
code snippet Christoph pointed out. Sounds like it'd be better to fix
the usage than the structure alignment.
Alex
On Wed, 09 Jun 2004 14:29:36 -0600
Alex Williamson <[email protected]> wrote:
> Which is probably why the patch never went anywhere. There's
> certainly an alignment issue in the usage of the struct arpt_arp in the
> code snippet Christoph pointed out. Sounds like it'd be better to fix
> the usage than the structure alignment.
Right. I distinctly remember a similar fix being needed to
ip_tables.c many months ago, a search though the change history
for that file might prove profitable :-)
On Wed, Jun 09, 2004 at 01:29:37PM -0700, David S. Miller wrote:
> On Wed, 09 Jun 2004 14:29:36 -0600
> Alex Williamson <[email protected]> wrote:
>
> > Which is probably why the patch never went anywhere. There's
> > certainly an alignment issue in the usage of the struct arpt_arp in the
> > code snippet Christoph pointed out. Sounds like it'd be better to fix
> > the usage than the structure alignment.
>
> Right. I distinctly remember a similar fix being needed to
> ip_tables.c many months ago, a search though the change history
> for that file might prove profitable :-)
Or alternatively look into the netfilter bugzilla at:
https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=84
If somebody wants to prepare a trivial merge of that fix with arptables
- it should be extermely straight forward ;)
--
- Harald Welte <[email protected]> http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime
On Wed, 2004-06-09 at 15:33, Harald Welte wrote:
> On Wed, Jun 09, 2004 at 01:29:37PM -0700, David S. Miller wrote:
> >
> > Right. I distinctly remember a similar fix being needed to
> > ip_tables.c many months ago, a search though the change history
> > for that file might prove profitable :-)
>
> Or alternatively look into the netfilter bugzilla at:
>
> https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=84
>
> If somebody wants to prepare a trivial merge of that fix with arptables
> - it should be extermely straight forward ;)
That change is probably appropriate, but IIRC, that's not the
alignment problem I saw, and that one certainly wouldn't have been fixed
by a change to the arpt_arp structure. Looking at the code snippet
again:
/* Look for ifname matches; this should unroll nicely. */
for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
ret |= (((const unsigned long *)indev)[i]
^ ((const unsigned long *)arpinfo->iniface)[i])
& ((const unsigned long *)arpinfo->iniface_mask)[i];
}
The alignment problem I remember was with iniface and iniface_mask. If
we can't change the structure alignment, the easiest fix is to change
the stride length to something appropriate for the arch or maybe just a
least common demoninator. Maybe someone is smart enough to get the
preprocessor to figure this out automatically... dunno if that's
possible.
While we're on this little piece of code, there's another bug here.
ret is defined as an int in arp_packet_match() so we're losing the upper
half of the result anyway. ip_packet_match() appears to already have
this correct. At a minimum, I think we need the trivial patch below.
Thanks,
Alex
===== net/ipv4/netfilter/arp_tables.c 1.13 vs edited =====
--- 1.13/net/ipv4/netfilter/arp_tables.c Sun Jun 6 21:15:04 2004
+++ edited/net/ipv4/netfilter/arp_tables.c Wed Jun 9 15:38:16 2004
@@ -106,7 +106,8 @@
char *arpptr = (char *)(arphdr + 1);
char *src_devaddr, *tgt_devaddr;
u32 *src_ipaddr, *tgt_ipaddr;
- int i, ret;
+ int i;
+ unsigned long ret;
#define FWINV(bool,invflg) ((bool) ^ !!(arpinfo->invflags & invflg))
On Wed, Jun 09, 2004 at 01:00:01PM -0700, David S. Miller wrote:
> How can you legitimately change this structure? It's an exported
> userland interface, if you change it all the applications will stop
> working.
Why not split the structure for user-space and kernel-space version
and cp/frob at/near the syscall boundary?
--cw
On Wed, Jun 09, 2004 at 06:45:19PM -0700, Chris Wedgwood wrote:
> On Wed, Jun 09, 2004 at 01:00:01PM -0700, David S. Miller wrote:
>
> > How can you legitimately change this structure? It's an exported
> > userland interface, if you change it all the applications will stop
> > working.
>
> Why not split the structure for user-space and kernel-space version
> and cp/frob at/near the syscall boundary?
because it would look like an ugly hack in the setsockopt call, plus
adding another costly/time consuming parse of the table BLOB.
Also note that the kernel currently has no code that supports the
generation/modification of rulesets. All it can do is iterate over them.
> --cw
--
- Harald Welte <[email protected]> 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
On Wed, 9 Jun 2004 11:09:42 -0700 (PDT)
Christoph Lameter <[email protected]> wrote:
> /* Look for ifname matches; this should unroll nicely. */
> for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
> ret |= (((const unsigned long *)indev)[i]
> ^ ((const unsigned long *)arpinfo->iniface)[i])
> & ((const unsigned long *)arpinfo->iniface_mask)[i];
> }
This is far from a critical code path, so this is how I'm
going to fix this.
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/06/10 22:05:19-07:00 [email protected]
# [IPV4]: Fix unaligned accesses in arp_tables.c
#
# net/ipv4/netfilter/arp_tables.c
# 2004/06/10 22:05:03-07:00 [email protected] +3 -4
# [IPV4]: Fix unaligned accesses in arp_tables.c
#
diff -Nru a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
--- a/net/ipv4/netfilter/arp_tables.c 2004-06-10 22:05:40 -07:00
+++ b/net/ipv4/netfilter/arp_tables.c 2004-06-10 22:05:40 -07:00
@@ -179,11 +179,10 @@
return 0;
}
- /* Look for ifname matches; this should unroll nicely. */
+ /* Look for ifname matches. */
for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
- ret |= (((const unsigned long *)indev)[i]
- ^ ((const unsigned long *)arpinfo->iniface)[i])
- & ((const unsigned long *)arpinfo->iniface_mask)[i];
+ ret |= (indev[i] ^ arpinfo->iniface[i])
+ & arpinfo->iniface_mask[i];
}
if (FWINV(ret != 0, ARPT_INV_VIA_IN)) {
On Jun 10, 2004 22:04 -0700, David S. Miller wrote:
> This is far from a critical code path, so this is how I'm
> going to fix this.
>
> # This is a BitKeeper generated diff -Nru style patch.
> #
> # ChangeSet
> # 2004/06/10 22:05:19-07:00 [email protected]
> # [IPV4]: Fix unaligned accesses in arp_tables.c
> #
> # net/ipv4/netfilter/arp_tables.c
> # 2004/06/10 22:05:03-07:00 [email protected] +3 -4
> # [IPV4]: Fix unaligned accesses in arp_tables.c
> #
> diff -Nru a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> --- a/net/ipv4/netfilter/arp_tables.c 2004-06-10 22:05:40 -07:00
> +++ b/net/ipv4/netfilter/arp_tables.c 2004-06-10 22:05:40 -07:00
> @@ -179,11 +179,10 @@
> return 0;
> }
>
> - /* Look for ifname matches; this should unroll nicely. */
> + /* Look for ifname matches. */
> for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
> - ret |= (((const unsigned long *)indev)[i]
> - ^ ((const unsigned long *)arpinfo->iniface)[i])
> - & ((const unsigned long *)arpinfo->iniface_mask)[i];
> + ret |= (indev[i] ^ arpinfo->iniface[i])
> + & arpinfo->iniface_mask[i];
> }
- for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
+ for (i = 0, ret = 0; i < IFNAMSIZ; i++) {
Shouldn't your change include the above?
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://members.shaw.ca/adilger/ http://members.shaw.ca/golinux/
On Thu, 10 Jun 2004 23:41:11 -0600
Andreas Dilger <[email protected]> wrote:
> - for (i = 0, ret = 0; i < IFNAMSIZ/sizeof(unsigned long); i++) {
> + for (i = 0, ret = 0; i < IFNAMSIZ; i++) {
>
> Shouldn't your change include the above?
Yes, I'm retarted, thanks for catching that.