[DTrace-devel] [PATCH 1/3] Add support for the link_ntop() subroutine

[Eugene Loh]

On 12/9/23 00:29, Kris Van Hees wrote:

> On Tue, Aug 29, 2023 at 09:54:31PM -0400, eugene.loh at oracle.com wrote:
>> diff --git a/bpf/link_ntop.S b/bpf/link_ntop.S
>> new file mode 100644
>> @@ -0,0 +1,127 @@
>> +#define ARPHRD_ETHER		1
>> +#define ARPHRD_INFINIBAND	32
> Maybe add that these coe from <linux/if_arp.h> ?

Maybe.  But that starts a cascade of other stuff that gets included.

In particular, if I understand correctly, we do not include 
/usr/include.  We include build/include, which has symlinks to various 
/usr/include/ directories.  The cascade leads to adding directories 
sys/, gnu/, bits/, etc., as well as files features.h, stdc-predef.h, 
etc.  We cannot just turn build/include into a link to /usr/include 
since then we could not touch .dir.stamp.  So, since we do not actually 
include /usr/include, it feels to me like these two #define make some 
sense.  Much simpler.

>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -6039,6 +6039,31 @@ dt_cg_subr_inet_ntop(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>   	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, tstring_offset));
>>   }
>>   
>> +/* Definitions from libdtrace/net.d or /usr/include/linux/if_arp.h. */
>> +#define ARPHRD_ETHER         1
>> +#define ARPHRD_INFINIBAND   32
> Why not simply include <linux/if_arp.h> ?

Yeah, though mimicking what's done in the bpf .S file also makes sense, 
and there (per earlier discussion) including if_arp.h gets complicated.

>> +
>> +static void
>> +dt_cg_subr_link_ntop(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>> +{
>> +	dt_node_t	*type = dnp->dn_args;
>> +	uint_t		Lokay = dt_irlist_label(dlp);
>> +
>> +	/* Determine type and check it is okay. */
>> +	dt_cg_node(type, dlp, drp);
>> +	emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, type->dn_reg, ARPHRD_INFINIBAND, Lokay));
>> +	emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, type->dn_reg, ARPHRD_ETHER, Lokay));
>> +	dt_cg_probe_error(yypcb, DTRACEFLT_ILLOP, DT_ISREG, type->dn_reg);
>> +	emitl(dlp, Lokay,
>> +		   BPF_NOP());
>> +
>> +	/* Move addr into dnp->dn_args so we can call dt_cg_subr_arg_to_tstring(). */
>> +	dnp->dn_args = type->dn_list;
> You probably should do this in a way that after the dt_cg_subr_arg_to_tstring()
> call, you can restore the node structure.  I.e. dnp->dn_args = type.  That way,
> depending on what type is, dt_node_free() will ensure proper cleanup is done
> when dt_node_free( is done on dnp.

Okay.

> But worse, this causes the dt_cg_check_ptr_arg() in dt_cg_subr_arg_to_tstring()
> to get bypassed (and thus err.link_ntopbadaddr.d fails) because you move the
> 2nd argument into the 1st argument spot, and then dt_cg_subr_arg_to_tstring()
> checks the type of the *1st* argument of link_type() according to its
> prototype.  And since that is not a pointer, the check is never done.

I don't follow.  dt_cg_subr_arg_to_tstring() checks the type of its 1st 
arg, which is the 2nd arg to link_ntop().  E.g., for 
err.link_ntopbadaddr.d, it's a pointer.  So that 2nd link_ntop() arg 
*is* a pointer, we do in fact run dt_cg_check_ptr_arg(), the NULL 
pointer is rejected, and the test passes.

Is err.link_ntopbadaddr.d really failing?

> Rather than remapping the args (exchanging type and addr), I think you need
> to keep them as they are, and do (assuming addr = dnp->dn_args):

How can we assume addr=dnp->dn_args?  The first arg of link_ntop() -- 
that is, dnp->dn_args -- is the type.

> 	dt_cg_node(addr, dlp, drp);
> 	dt_cg_check_ptr_arg(dlp, drp, addr, NULL);
> 	dt_cg_subr_arg_to_tstring(dnp, dlp, drp, "dt_link_ntop", DT_ISREG, addr->dn_reg, DT_IGNOR, 0);
>
> But then you do need to rework dt_link_ntop to accept type as the main argument
> and addr as the extra argument.
>
>> +
>> +	dt_cg_subr_arg_to_tstring(dnp, dlp, drp, "dt_link_ntop", DT_ISREG, type->dn_reg);
>> +	dt_regset_free(drp, type->dn_reg);
>> +}