[DTrace-devel] [PATCH 7/7] Add support for inet_ntoa6() subroutine

[Eugene Loh]

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:

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.