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

[Kris Van Hees]

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).

> 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.

I think including if_arp.h here makes more sense.

> > > +
> > > +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.

> 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