[DTrace-devel] [PATCH 2/2] Add support for inet_ntop() subroutine

Kris Van Hees kris.van.hees at oracle.com
Tue Sep 12 20:14:59 UTC 2023


On Mon, Aug 28, 2023 at 03:08:00PM -0400, eugene.loh at oracle.com wrote:
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  libdtrace/dt_cg.c                             |  85 +++++++++++++-
>  test/unittest/funcs/tst.inet_ntop.d           |   1 -
>  .../unittest/funcs/tst.inet_ntop_runtime_af.d | 104 ++++++++++++++++++
>  .../unittest/funcs/tst.inet_ntop_runtime_af.r |  12 ++
>  4 files changed, 200 insertions(+), 2 deletions(-)
>  create mode 100644 test/unittest/funcs/tst.inet_ntop_runtime_af.d
>  create mode 100644 test/unittest/funcs/tst.inet_ntop_runtime_af.r
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index afd272d7..bb73efd7 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -5956,6 +5956,89 @@ dt_cg_subr_inet_ntoa6(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>  				  DT_ISIMM, tbloff);
>  }
>  
> +static void
> +dt_cg_subr_inet_ntop(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> +	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
> +	dt_node_t	*af = dnp->dn_args;
> +	dt_node_t	*addr = af->dn_list;
> +	uint_t		Lipv6 = dt_irlist_label(dlp);
> +	uint_t		Ldone = dt_irlist_label(dlp);
> +	uint64_t	tstring_offset;
> +
> +	/* Determine address family af. */
> +	dt_cg_node(af, dlp, drp);
> +
> +	/* Move addr into dnp->dn_args so we can call inet_ntoa[6]. */
> +	dnp->dn_args = addr;
> +
> +	/* Execute a runtime branch depending on af. */
> +	emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, af->dn_reg, AF_INET6, Lipv6));
> +
> +	/*
> +	 * The code generator needs to know where the results will be.
> +	 * So allocate somewhere for the results.  Each runtime branch
> +	 * will copy to this location.
> +	 */
> +	tstring_offset = dt_cg_tstring_xalloc(yypcb);
> +
> +	/* The ipv4 case. */
> +	dt_cg_subr_inet_ntoa(dnp, dlp, drp);
> +
> +	/* Copy the results out. */
> +	if (dt_regset_xalloc_args(drp) == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, dtp->dt_options[DTRACEOPT_STRSIZE]));
> +	emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> +	emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_MEM));
> +	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, tstring_offset));
> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> +	dt_regset_free(drp, BPF_REG_0);
> +	dt_regset_free_args(drp);
> +
> +	/* Free resources. */
> +	dt_regset_free(drp, dnp->dn_reg);
> +	dt_cg_tstring_free(yypcb, dnp);         // FIXME but dnp->dn_tstring is still there?
> +
> +	/* Jump to end. */
> +	emit(dlp,  BPF_JUMP(Ldone));
> +
> +	/* The ipv6 case. */
> +	emitl(dlp, Lipv6,
> +		   BPF_NOP());
> +	dt_cg_subr_inet_ntoa6(dnp, dlp, drp);
> +
> +	/* Copy the results out. */
> +	if (dt_regset_xalloc_args(drp) == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, dtp->dt_options[DTRACEOPT_STRSIZE]));
> +	emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> +	emit(dlp,  BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_1, DCTX_MEM));
> +	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, tstring_offset));
> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> +	dt_regset_free(drp, BPF_REG_0);
> +	dt_regset_free_args(drp);
> +
> +	/* Free resources. */
> +	dt_regset_free(drp, dnp->dn_reg);
> +	dt_cg_tstring_free(yypcb, dnp);         // FIXME but dnp->dn_tstring is still there?
> +
> +	/* Done with both branches.  Save the results to dnp. */
> +	emitl(dlp, Ldone,
> +		   BPF_NOP());
> +
> +	/* We still have af->dn_reg, so use it for dnp. */
> +	dnp->dn_reg = af->dn_reg;
> +	dnp->dn_tstring->dn_value = tstring_offset;
> +	emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, DT_STK_DCTX));
> +	emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, dnp->dn_reg, DCTX_MEM));
> +	emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, tstring_offset));

This code makes dangerous modifications to dnp that can cause memory leaks and
other potential issues.  E.g. you overwrite dnp->dn_args which holds af, so
after that dnp->dn_args points to addr and no ref to af is retained.  That can
affect cleanup later before af has become an orphaned node. 

You also end up allocating the result tstring earlier than necessary as far as
I can see.

Since we already have code that essentially does the same thing (ternary
expression with string values), perhaps implement this the same way?  You could
duplicate the code (which is perhaps not the best choice here) or construct an
OP3 node with af == AF_INET6 as expr, and FUNC nodes for lp and rp (calls to
inet_ntoa6() and inet_ntoa() respectively).  And then just call
dt_cg_ternary_op() to generate the actual code?

Going the dt_cg_ternary_op() route also has the advantage that if/when (and I
hope soon) we implement optimization of ternary expressions with a constant
condition, this will benefit from that change right away as well rather than
needing its own optimization (if we care).

> +}
> +
>  typedef void dt_cg_subr_f(dt_node_t *, dt_irlist_t *, dt_regset_t *);
>  
>  static dt_cg_subr_f *_dt_cg_subr[DIF_SUBR_MAX + 1] = {
> @@ -6000,7 +6083,7 @@ static dt_cg_subr_f *_dt_cg_subr[DIF_SUBR_MAX + 1] = {
>  	[DIF_SUBR_NTOHS]		= &dt_cg_subr_htons,
>  	[DIF_SUBR_NTOHL]		= &dt_cg_subr_htonl,
>  	[DIF_SUBR_NTOHLL]		= &dt_cg_subr_htonll,
> -	[DIF_SUBR_INET_NTOP]		= NULL,
> +	[DIF_SUBR_INET_NTOP]		= &dt_cg_subr_inet_ntop,
>  	[DIF_SUBR_INET_NTOA]		= &dt_cg_subr_inet_ntoa,
>  	[DIF_SUBR_INET_NTOA6]		= &dt_cg_subr_inet_ntoa6,
>  	[DIF_SUBR_D_PATH]		= NULL,
> diff --git a/test/unittest/funcs/tst.inet_ntop.d b/test/unittest/funcs/tst.inet_ntop.d
> index 234effb2..e2f00356 100644
> --- a/test/unittest/funcs/tst.inet_ntop.d
> +++ b/test/unittest/funcs/tst.inet_ntop.d
> @@ -4,7 +4,6 @@
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
>  
>  #pragma D option quiet
>  
> diff --git a/test/unittest/funcs/tst.inet_ntop_runtime_af.d b/test/unittest/funcs/tst.inet_ntop_runtime_af.d
> new file mode 100644
> index 00000000..7843a4ea
> --- /dev/null
> +++ b/test/unittest/funcs/tst.inet_ntop_runtime_af.d
> @@ -0,0 +1,104 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2007, 2023, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * Check that inet_ntop(af, addr) works even if af is not known until runtime.
> + */
> +
> +#pragma D option quiet
> +
> +ipaddr_t *ip4a;
> +ipaddr_t *ip4b;
> +ipaddr_t *ip4c;
> +ipaddr_t *ip4d;
> +struct in6_addr *ip6a;
> +struct in6_addr *ip6b;
> +struct in6_addr *ip6c;
> +struct in6_addr *ip6d;
> +struct in6_addr *ip6e;
> +struct in6_addr *ip6f;
> +struct in6_addr *ip6g;
> +
> +BEGIN
> +{
> +	this->buf4a = alloca(sizeof(ipaddr_t));
> +	this->buf4b = alloca(sizeof(ipaddr_t));
> +	this->buf4c = alloca(sizeof(ipaddr_t));
> +	this->buf4d = alloca(sizeof(ipaddr_t));
> +	this->buf6a = alloca(sizeof(struct in6_addr));
> +	this->buf6b = alloca(sizeof(struct in6_addr));
> +	this->buf6c = alloca(sizeof(struct in6_addr));
> +	this->buf6d = alloca(sizeof(struct in6_addr));
> +	this->buf6e = alloca(sizeof(struct in6_addr));
> +	this->buf6f = alloca(sizeof(struct in6_addr));
> +	this->buf6g = alloca(sizeof(struct in6_addr));
> +	ip4a = this->buf4a;
> +	ip4b = this->buf4b;
> +	ip4c = this->buf4c;
> +	ip4d = this->buf4d;
> +	ip6a = this->buf6a;
> +	ip6b = this->buf6b;
> +	ip6c = this->buf6c;
> +	ip6d = this->buf6d;
> +	ip6e = this->buf6e;
> +	ip6f = this->buf6f;
> +	ip6g = this->buf6g;
> +
> +	*ip4a = htonl(0xc0a80117);
> +	*ip4b = htonl(0x7f000001);
> +	*ip4c = htonl(0xffffffff);
> +	*ip4d = htonl(0x00000000);
> +
> +	ip6a->in6_u.u6_addr8[0] = 0xfe;
> +	ip6a->in6_u.u6_addr8[1] = 0x80;
> +	ip6a->in6_u.u6_addr8[8] = 0x02;
> +	ip6a->in6_u.u6_addr8[9] = 0x14;
> +	ip6a->in6_u.u6_addr8[10] = 0x4f;
> +	ip6a->in6_u.u6_addr8[11] = 0xff;
> +	ip6a->in6_u.u6_addr8[12] = 0xfe;
> +	ip6a->in6_u.u6_addr8[13] = 0x0b;
> +	ip6a->in6_u.u6_addr8[14] = 0x76;
> +	ip6a->in6_u.u6_addr8[15] = 0xc8;
> +	ip6b->in6_u.u6_addr8[0] = 0x10;
> +	ip6b->in6_u.u6_addr8[1] = 0x80;
> +	ip6b->in6_u.u6_addr8[10] = 0x08;
> +	ip6b->in6_u.u6_addr8[11] = 0x08;
> +	ip6b->in6_u.u6_addr8[13] = 0x20;
> +	ip6b->in6_u.u6_addr8[13] = 0x0c;
> +	ip6b->in6_u.u6_addr8[14] = 0x41;
> +	ip6b->in6_u.u6_addr8[15] = 0x7a;
> +	ip6c->in6_u.u6_addr8[15] = 0x01;
> +	ip6e->in6_u.u6_addr8[12] = 0x7f;
> +	ip6e->in6_u.u6_addr8[15] = 0x01;
> +	ip6f->in6_u.u6_addr8[10] = 0xff;
> +	ip6f->in6_u.u6_addr8[11] = 0xff;
> +	ip6f->in6_u.u6_addr8[12] = 0x7f;
> +	ip6f->in6_u.u6_addr8[15] = 0x01;
> +	ip6g->in6_u.u6_addr8[10] = 0xff;
> +	ip6g->in6_u.u6_addr8[11] = 0xfe;
> +	ip6g->in6_u.u6_addr8[12] = 0x7f;
> +	ip6g->in6_u.u6_addr8[15] = 0x01;
> +
> +	v = 4;
> +	af = (v == 4 ? AF_INET : (v == 6 ? AF_INET6 : -1));
> +	printf("%s\n", inet_ntop(af, ip4a));
> +	printf("%s\n", inet_ntop(af, ip4b));
> +	printf("%s\n", inet_ntop(af, ip4c));
> +	printf("%s\n", inet_ntop(af, ip4d));
> +
> +	v = 6;
> +	af = (v == 4 ? AF_INET : (v == 6 ? AF_INET6 : -1));
> +	printf("%s\n", inet_ntop(af, ip6a));
> +	printf("%s\n", inet_ntop(af, ip6b));
> +	printf("%s\n", inet_ntop(af, ip6c));
> +	printf("%s\n", inet_ntop(af, ip6d));
> +	printf("%s\n", inet_ntop(af, ip6e));
> +	printf("%s\n", inet_ntop(af, ip6f));
> +	printf("%s\n", inet_ntop(af, ip6g));
> +
> +	exit(0);
> +}
> diff --git a/test/unittest/funcs/tst.inet_ntop_runtime_af.r b/test/unittest/funcs/tst.inet_ntop_runtime_af.r
> new file mode 100644
> index 00000000..913eff77
> --- /dev/null
> +++ b/test/unittest/funcs/tst.inet_ntop_runtime_af.r
> @@ -0,0 +1,12 @@
> +192.168.1.23
> +127.0.0.1
> +255.255.255.255
> +0.0.0.0
> +fe80::214:4fff:fe0b:76c8
> +1080::808:c:417a
> +::1
> +::
> +127.0.0.1
> +127.0.0.1
> +::fffe:7f00:1
> +
> -- 
> 2.18.4
> 
> 



More information about the DTrace-devel mailing list