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

[Eugene Loh]

On 12/13/23 00:03, Eugene Loh wrote:

> 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/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>>>>> +
>>>>> +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.

I posted the v2.

Note:  This patch was originally 1/3 (the patch and then two trivial, 
mildly related changes).  I think patches 2 and 3 are still awaiting 
review.  The v2 of this patch is preceded by two other patches.  So, v1 
was 1/3, while v2 is 3/3 (the third of a 3-patch series).

>>> 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);
>>>>> +}