[DTrace-devel] [PATCH 7/7] Add support for inet_ntoa6() subroutine
Kris Van Hees
kris.van.hees at oracle.com
Wed Aug 2 05:45:21 UTC 2023
On Tue, Aug 01, 2023 at 07:22:01PM -0400, Eugene Loh via DTrace-devel wrote:
> On 8/1/23 16:57, Kris Van Hees wrote:
>
> > On Tue, Aug 01, 2023 at 01:40:40AM -0400, Eugene Loh via DTrace-devel wrote:
> >
> > > Also, it'd be great if the code had some high-level roadmap of where this
> > > very difficult minimal-branching implementation is going. Hopefully, it's
> > > 100% correct and no one will ever have to look at this code again.
> > > Otherwise, it's a nightmare. Maybe there could be some C code in the
> > > comments that illustrates what's going on in the BPF where each C statement
> > > maps to a set of BPF instructions, even if the branch-free BPF instructions
> > > take some head-scratching to figure out. E.g., "d = (c || s) ? 1 : 0" maps
> > > to "add %r5, %r0; neg %r5; rsh %r5, 63".
> > The problem with C code is that it is about as obscure as this assembly code
> > unless you write it as a higher level implementation, which then again will
> > show branches and therefore become less useful.
>
> Actually "show branches" is kind of what I meant. Show the higher-level
> implementation. People can then separate "is that doing what inet_ntoa6
> should do?" from "how do the BPF instructions accomplish that?" Anyhow,
> just a suggestion. Ultimately, the bottom line here is that this stuff is
> just plain tricky
>
> > I'll add a bit more commentary to explain the use of the bitmap of non-zero
> > words and the lookup table.
> >
>
> Cool.
>
> > > Maybe it should also acknowledge that there are subtle ways that the
> > > implemented algorithm differs from legacy DTrace (dtrace_dif.c). Namely:
> > > *) There seem to be differences in the ways the two implementations decide
> > > between ipv4 and v6. I don't understand this stuff well enough to judge if
> > > there are any errors here.
> > I do not see where there are differences between legacy and this version when
> > it comes to IPv4 and IPv6 addresses. As far as I can see, the behaviour for
> > mapped and embedded IPv4 addresses is the sme.
>
> I have no idea if these differences are important, but they're there. One
> can see that by examining the code, but that requires some poking around
> into ipv6_addr_type(). So another way is just to try some "random" inputs:
The problem is that there is no canonical way to make a determination here.
One interpretation is that anything that has 6 leading zero-words is an IPv4
address. But that is not true if the next two words are both 0 (unspecified
address) or the one before last is 0 and the last is 1 (loopback). So that
already shows this rule is not adequate.
Now, RFC 4291 states that the IPv4 address in an IPv6 address must be 'a
globally-unique IPv4 unicast address'. And valid IPv4 address must have a
first octet that is not zero. So, it can be argued that if the last two words
di not represent a valid IPv4 address, we do not try to interpret it as such.
It is easy enough to make it consistent with the legacy version though so might
as well.
> struct in6_addr *ip6;
> BEGIN
> {
> ip6 = alloca(sizeof (struct in6_addr));
> ip6->in6_u.u6_addr8[ 0] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 0] = 0;
> ip6->in6_u.u6_addr8[ 1] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 1] = 0;
> ip6->in6_u.u6_addr8[ 2] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 2] = 0;
> ip6->in6_u.u6_addr8[ 3] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 3] = 0;
> ip6->in6_u.u6_addr8[ 4] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 4] = 0;
> ip6->in6_u.u6_addr8[ 5] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 5] = 0;
> ip6->in6_u.u6_addr8[ 6] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 6] = 0;
> ip6->in6_u.u6_addr8[ 7] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 7] = 0;
> ip6->in6_u.u6_addr8[ 8] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 8] = 0;
> ip6->in6_u.u6_addr8[ 9] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[ 9] = 0;
> ip6->in6_u.u6_addr8[10] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[10] = 0;
> ip6->in6_u.u6_addr8[11] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[11] = 0;
> ip6->in6_u.u6_addr8[12] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[12] = 0;
> ip6->in6_u.u6_addr8[13] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[13] = 0;
> ip6->in6_u.u6_addr8[14] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[14] = 0;
> ip6->in6_u.u6_addr8[15] = 0x01; printf("%s\n", inet_ntoa6(ip6));
> ip6->in6_u.u6_addr8[15] = 0;
> exit(0);
> }
>
> With DTv1, I get:
>
> 100::
> 1::
> 0:100::
> 0:1::
> 0:0:100::
> 0:0:1::
> 0:0:0:100::
> 0:0:0:1::
> ::100:0:0:0
> ::1:0:0:0
> ::100:0:0
> ::1:0:0
> 1.0.0.0
> 0.1.0.0
> 0.0.1.0
> ::1
>
> With the proposed patch:
>
> 100::
> 1::
> 0:100::
> 0:1::
> 0:0:100::
> 0:0:1::
> 0:0:0:100::
> 0:0:0:1::
> ::100:0:0:0
> ::1:0:0:0
> ::100:0:0
> ::1:0:0
> 1.0.0.0
> 0.1.0.0
> ::100
> ::1
>
> Lots of agreement, but the second-to-last result differs. I'm not saying
> these are meaningful -- or even legal -- cases, but only that one can see
> that the selection is not the same between the two implementations. There
> are other examples.
>
> > > This patch uses bpf/inet_ntoa.S. That file claims the function is void, but
> > > inet_ntoa6() uses the return value. So, that file should document the
> > > function correctly. Also, that file claims to fill with ":". It actually
> > > fills with ".".
> > The code now no longer expects inet_ntoa() to return a value, so this no
> > longer matters. I dropped the patch adding a return value from the series.
> >
> > If bpf/inet_ntoa.S documents is behaviour wrong (i.e. you point out how your
> > comment claim ':' as separator whereas the implementation correctly uses '.'),
> > you should subnmit a patch to fix that. Or I can do it - but outside this
> > series because it is independent.
>
> Yeah. My fault in the first place. I can submit a patch.
>
> > > On 7/27/23 11:31, Kris Van Hees via DTrace-devel wrote:
> > > > diff --git a/bpf/inet_ntoa6.S b/bpf/inet_ntoa6.S
> > > > +#define DCTX_RODATA 56
> > > 56 hardwired?
> > The trouble is that assembler input files cannot contain struct declarations
> > and use offsetof(), so the only way to avoid this is to add a utility that
> > writes out a .h file at build time to provide constants like this. Worth
> > considering - though since this is the only occurence (for now) maybe a
> > comment might suffice?
>
> Sure.
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list