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

[Eugene Loh]

On 12/11/23 23:39, Kris Van Hees wrote:

> On Mon, Dec 11, 2023 at 05:03:05PM -0500, Eugene Loh via DTrace-devel wrote:
>> 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.
> I didn't say include linux/if_arp.h...  just mention that those constants are
> taken from that include file (like you do for the following ones).

Got it.  Okay, I can see that's what you said originally.  My confusion.

>>>> 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.
> I think including if_arp.h here makes more sense.

Okay.

>>>> +
>>>> +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.
> The type is checked based on what the prototype of link_ntop() provides, not
> based on the type of the passed argument.  See dt_cg_subr_arg_to_tstring():
>
>          isp = dnp->dn_ident->di_data;
>          assert(isp && isp->dis_args);
>          argtype = &isp->dis_args[0];
>
>          /* handle the one "input value" */
>          /* (its type matters only as to whether we check it is null */
>          dt_cg_node(arg, dlp, drp);
>          if (dt_node_is_pointer(argtype) || dt_node_is_string(argtype))
>                  dt_cg_check_ptr_arg(dlp, drp, arg, NULL);
>
> So, in this case you pass the 2nd argument of link_ntop(), but the check is
> done on the expected type of the first argument of link_ntop() which is an
> integer.

Ah, "new" code (that I reviewed!) since the patch was first posted. 
Which explains why the badaddr test started failing.

Here's an alternative proposal.  dt_cg_subr_arg_to_tstring() kind of 
takes a subroutine arg, does ptr checks, and converts it to a tstring by 
calling a BPF function.  It can pass other arguments to the BPF 
function, and it is sometimes used for subroutines with more than one 
arg, but something feels "minor" about those other args. FWIW, 
dt_cg_subr_arg_to_tstring() claims to limit them to 32-bit ints.

So how about I add another arg to dt_cg_subr_arg_to_tstring() giving the 
arg number of the subroutine where to grab the main arg. Usually, this 
is going to be 0.  For link_ntop(int, void*), however, we just say that 
the arg number is 1:  we want the second subroutine arg.

This means that dt_cg_subr_link_ntop() doesn't need to vet the string 
ptr or juggle args around.  And dt_cg_subr_arg_to_tstring() just needs 
to access dis_args[argnum] instead of just dis_args[0]. And chase 
"while(argnum-- > 0) arg = arg->dn_list;".

I have a modified patch, but wanted to run the idea past you before 
posting a v2.  Or, I can just post the v2 if you like.

>> Is err.link_ntopbadaddr.d really failing?
> Yes.
>
>>> 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.
> Sorry, my mistake.  But you know what I mean by addr - the actual HW address.
>
>>> 	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);
>>>> +}
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel