Re: "Wire" definitions and __packed

[Joerg Sonnenberger]

On Thu, Oct 06, 2016 at 06:08:30PM +, paul.kon...@dell.com wrote:
> Still, though, the original comment is largely valid: you can't do
> meaningful testing of changes that affect alignment on an x86 system,
> because for the most part it doesn't care.  (The same goes for various
> other CISC machines such as VAX.)  Also, structure padding is different
> for x86 than for most RISC machines.

Can we please the keep the amount of generic wisdom injection down,
please? Roy is quite aware that x86 is not strict alignment. As I said
in my initial mail, the proposed changes have a chance of exposing
incorrect alignment assumptions in the kernel. Some of them will be
properly detected at build time, some will only be found due to testing.
Padding differences are irrelevant since I explicitly said that size
assertions are going to be kept. Random general remarks do nothing to
improve the situation.

Joerg

Re: "Wire" definitions and __packed

[Paul.Koning]

> On Oct 6, 2016, at 2:01 PM, Joerg Sonnenberger  wrote:
> 
> On Fri, Oct 07, 2016 at 04:59:30AM +1100, matthew green wrote:
>> John Nemeth writes:
>>> On Oct 6,  3:01pm, matthew green wrote:
>>> }
>>> } >  X86 doesn't have alignment restrictions.  The platform
>>> } > practically lets you get away with murder, and thus is not useful
>>> } > as a test platform.
>>> } 
>>> } FWIW, this hasn't been true since at least 1999 (SSE.)  also,
>>> 
>>> That only counts if somebody is using SSE, and I highly doubt
>>> that dhcpcd does.
>> 
>> GCC will emit SSE code even if you don't explicitly use them.
> 
> Like for inlined memset or memcpy...

Still, though, the original comment is largely valid: you can't do meaningful 
testing of changes that affect alignment on an x86 system, because for the most 
part it doesn't care.  (The same goes for various other CISC machines such as 
VAX.)  Also, structure padding is different for x86 than for most RISC machines.

The trouble with making a change in fundamental machinery and then doing a 
"test it to see if it breaks" is that this only exposes issues in the code 
paths that happened to be touched by the particular test.

paul

Re: "Wire" definitions and __packed

[Joerg Sonnenberger]

On Fri, Oct 07, 2016 at 04:59:30AM +1100, matthew green wrote:
> John Nemeth writes:
> > On Oct 6,  3:01pm, matthew green wrote:
> > }
> > } >  X86 doesn't have alignment restrictions.  The platform
> > } > practically lets you get away with murder, and thus is not useful
> > } > as a test platform.
> > } 
> > } FWIW, this hasn't been true since at least 1999 (SSE.)  also,
> > 
> >  That only counts if somebody is using SSE, and I highly doubt
> > that dhcpcd does.
> 
> GCC will emit SSE code even if you don't explicitly use them.

Like for inlined memset or memcpy...

Joerg

re: "Wire" definitions and __packed

[matthew green]

John Nemeth writes:
> On Oct 6,  3:01pm, matthew green wrote:
> }
> } >  X86 doesn't have alignment restrictions.  The platform
> } > practically lets you get away with murder, and thus is not useful
> } > as a test platform.
> } 
> } FWIW, this hasn't been true since at least 1999 (SSE.)  also,
> 
>  That only counts if somebody is using SSE, and I highly doubt
> that dhcpcd does.

GCC will emit SSE code even if you don't explicitly use them.
that's why we build kernels etc. with -mno-sse -mno-mmx, etc.
it could also be using an SSE enhanced functionality in libc.

you can never really be sure unless you control the entire
environment (like a kernel.)


.mrg.

re: "Wire" definitions and __packed

[John Nemeth]

On Oct 6,  3:01pm, matthew green wrote:
}
} >  X86 doesn't have alignment restrictions.  The platform
} > practically lets you get away with murder, and thus is not useful
} > as a test platform.
} 
} FWIW, this hasn't been true since at least 1999 (SSE.)  also,

 That only counts if somebody is using SSE, and I highly doubt
that dhcpcd does.

} while no one uses them, x86 has "alignment checking" options.

 I am aware of the flag, but as you noted nobody uses it, thus
it might as well not be there.

}-- End of excerpt from matthew green

re: "Wire" definitions and __packed

[matthew green]

>  X86 doesn't have alignment restrictions.  The platform
> practically lets you get away with murder, and thus is not useful
> as a test platform.

FWIW, this hasn't been true since at least 1999 (SSE.)  also,
while no one uses them, x86 has "alignment checking" options.


.mrg.

Re: "Wire" definitions and __packed

[John Nemeth]

On Oct 5, 10:15pm, Roy Marples wrote:
} On Wednesday 05 October 2016 17:10:28 Eduardo Horvath wrote:
} > On Wed, 5 Oct 2016, Roy Marples wrote:
} > > On 04/10/2016 23:06, Joerg Sonnenberger wrote:
} > > > I'd like to addressing this by cutting down on the first set. For this
} > > > purpose, I want to replace many of the __packed attributes in the
} > > > current network headers with CTASSERT of the proper size, especially for
} > > > those structs that are clearly not wire definitions by themselve.
} > > 
} > > I tested the following structs without packed with the latest dhcpcd
} > > trunk (not yet in NetBSD).
} > > 
} > > ip
} > > udphdr
} > > arphdr
} > > in_addr
} > > nd_router_advert
} > > nd_opt_hdr
} > > nd_opt_prefix_info
} > > nd_opt_mtu
} > > nd_opt_rdnss
} > > nd_opt_dnssl
} > > 
} > > Works fine so far.
} > 
} > What platforms did you test it on?
} > 
} > I recommend trying it on sparc64.  That's one of the worst cases, being
} > big-endian 64-bit with alignment constraints.  And I recall some ABI (was
} > it ARM?) has strange alignment restrictions on byte values.
} 
} i386/amd64 only right now.

 X86 doesn't have alignment restrictions.  The platform
practically lets you get away with murder, and thus is not useful
as a test platform.

} I'll test on mips64-eb tomorrow.
} Sadly my sparc64 is dead, the network card reports an unspecified hardware 
} address.

 Traditionally, sparc boxes got their network MAC address
programmed by a value specified in CMOS RAM.  This likely means
that the CMOS battery is dead.

}-- End of excerpt from Roy Marples

Re: "Wire" definitions and __packed

[Roy Marples]

On Wednesday 05 October 2016 17:10:28 Eduardo Horvath wrote:
> On Wed, 5 Oct 2016, Roy Marples wrote:
> > On 04/10/2016 23:06, Joerg Sonnenberger wrote:
> > > I'd like to addressing this by cutting down on the first set. For this
> > > purpose, I want to replace many of the __packed attributes in the
> > > current network headers with CTASSERT of the proper size, especially for
> > > those structs that are clearly not wire definitions by themselve.
> > 
> > I tested the following structs without packed with the latest dhcpcd
> > trunk (not yet in NetBSD).
> > 
> > ip
> > udphdr
> > arphdr
> > in_addr
> > nd_router_advert
> > nd_opt_hdr
> > nd_opt_prefix_info
> > nd_opt_mtu
> > nd_opt_rdnss
> > nd_opt_dnssl
> > 
> > Works fine so far.
> 
> What platforms did you test it on?
> 
> I recommend trying it on sparc64.  That's one of the worst cases, being
> big-endian 64-bit with alignment constraints.  And I recall some ABI (was
> it ARM?) has strange alignment restrictions on byte values.

i386/amd64 only right now.
I'll test on mips64-eb tomorrow.
Sadly my sparc64 is dead, the network card reports an unspecified hardware 
address.

Roy

Re: "Wire" definitions and __packed

[Joerg Sonnenberger]

On Wed, Oct 05, 2016 at 05:10:28PM +, Eduardo Horvath wrote:
> I recommend trying it on sparc64.  That's one of the worst cases, being 
> big-endian 64-bit with alignment constraints.  And I recall some ABI (was 
> it ARM?) has strange alignment restrictions on byte values.

Old (ancient?) ARM ABIs had a minimal struct size. Thankfully, noone is
brain dead to do something like that and in fact, we had disabled that
part from what I dimly remember. We do assume that power-of-two sized
types require no more than that in terms of alignment. While not
strictly guaranteed by the standard, I believe a "don't care" attitude
is justified for platforms don't follow that.

Joerg

Re: "Wire" definitions and __packed

[Eduardo Horvath]

On Wed, 5 Oct 2016, Roy Marples wrote:

> On 04/10/2016 23:06, Joerg Sonnenberger wrote:
> > I'd like to addressing this by cutting down on the first set. For this
> > purpose, I want to replace many of the __packed attributes in the
> > current network headers with CTASSERT of the proper size, especially for
> > those structs that are clearly not wire definitions by themselve.
> 
> I tested the following structs without packed with the latest dhcpcd
> trunk (not yet in NetBSD).
> 
> ip
> udphdr
> arphdr
> in_addr
> nd_router_advert
> nd_opt_hdr
> nd_opt_prefix_info
> nd_opt_mtu
> nd_opt_rdnss
> nd_opt_dnssl
> 
> Works fine so far.

What platforms did you test it on?

I recommend trying it on sparc64.  That's one of the worst cases, being 
big-endian 64-bit with alignment constraints.  And I recall some ABI (was 
it ARM?) has strange alignment restrictions on byte values.

Eduardo

Re: "Wire" definitions and __packed

[Roy Marples]

On 04/10/2016 23:06, Joerg Sonnenberger wrote:
> I'd like to addressing this by cutting down on the first set. For this
> purpose, I want to replace many of the __packed attributes in the
> current network headers with CTASSERT of the proper size, especially for
> those structs that are clearly not wire definitions by themselve.

I tested the following structs without packed with the latest dhcpcd
trunk (not yet in NetBSD).

ip
udphdr
arphdr
in_addr
nd_router_advert
nd_opt_hdr
nd_opt_prefix_info
nd_opt_mtu
nd_opt_rdnss
nd_opt_dnssl

Works fine so far.

Roy

"Wire" definitions and __packed

[Joerg Sonnenberger]

Hello all,
a long time ago, Jason Thorpe added the packed attribute to a lot of
"wire" format definitions. Some of those like in_addr are part of a lot
of other interfaces that have nothing to do with on-wire
representations. The attribute has two functions:
(1) It removes any implicit padding as expected from the name.
(2) It also (unspectedly for many) removes any alignment constraints
from the structure.

The second item is problematic as can result in rather inefficient
byte-wise access. More important and the reason why I am bringing this
up: it impacts addresses to individual struct members. The current clang
development version has a new warning to trigger for situations like

   struct foo {
 int x;
   } __packed;

   int *f(struct foo *f) {
 return 
   }

The return value of f() is not guarenteed to be aligned correctly, so
this is effectively an implicit cast that raises the alignment. The
clang warning is still being refined to avoid triggering in situation
where alignment is explicitly not relevant (cast to intptr_t, cast to
char *, cast to void *) or where other alignment constraints are still
in place. Nevertheless quite a few cases arround the network stack
remain, triggering this warning. While investigating them, I have found
two different situations. Some places are safe and there should not have
been a __packed in first place. in_addr is a good example where the
attribute servers no real purpose. For other places it is not easy to
see where the alignment guarantees come from, so whether they work on
strict alignment platforms is difficult to tell.

I'd like to addressing this by cutting down on the first set. For this
purpose, I want to replace many of the __packed attributes in the
current network headers with CTASSERT of the proper size, especially for
those structs that are clearly not wire definitions by themselve. There
is a certain chance that some code path in the kernel currently does
depend on the implicit byte access to work, so this is not without risk.
On the other hand, it can result in more efficient code and better
diagnostic of mismatches, so IMO it will be an over all win. It should
also make it easier to identify cases where we do mishandled unaligned
access. Comments?

Joerg