[DTrace-devel] [PATCH v5] Add support for uregs[]

[Kris Van Hees]

On Sun, Feb 26, 2023 at 09:02:16PM -0500, Eugene Loh via DTrace-devel wrote:
> On 2/26/23 19:55, Kris Van Hees wrote:
> 
> > On Sun, Feb 26, 2023 at 07:45:40PM -0500, eugene.loh--- via DTrace-devel wrote:
> > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > index 9f0675fa..ebd9a81c 100644
> > > --- a/libdtrace/dt_cg.c
> > > +++ b/libdtrace/dt_cg.c
> > > @@ -3944,6 +3944,92 @@ dt_cg_assoc_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > >   	TRACE_REGSET("    assoc_op: End  ");
> > >   }
> > > +static void
> > > +dt_cg_uregs(unsigned int idx, dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > > +{
> > > +	dtrace_hdl_t *dtp = yypcb->pcb_hdl;
> > > +
> > > +	/* check if out-of-bounds */
> > > +	if (idx >= sizeof(dt_pt_regs) / sizeof(uint64_t)) {
> > > +
> > > +#if defined(__amd64)
> > > +		/*
> > > +		 * Even if out-of-bounds, on x86 there are still a few
> > > +		 * indices that are used to access task->thread. members.
> > > +		 */
> > > +		if (idx <= 25) {
> > > +			ctf_file_t *cfp = dtp->dt_shared_ctf;
> > > +			ctf_id_t type;
> > > +			ctf_membinfo_t ctm;
> > > +			int offset, rc;
> > > +
> > > +			/* look up task->thread offset */
> > > +			if (!cfp)
> > > +				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> > > +
> > > +			type = ctf_lookup_by_name(cfp, "struct task_struct");
> > > +			if (type == CTF_ERR)
> > > +				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> > > +			if (ctf_member_info(cfp, type, "thread", &ctm) == CTF_ERR)
> > > +				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> > > +			offset = ctm.ctm_offset / NBBY;
> > > +
> > > +			/* add the thread->member offset */
> > > +			type = ctf_lookup_by_name(cfp, "struct thread_struct");
> > > +			if (type == CTF_ERR)
> > > +				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> > > +			switch (idx) {
> > > +			case 21: rc = ctf_member_info(cfp, type, "ds", &ctm); break;
> > > +			case 22: rc = ctf_member_info(cfp, type, "es", &ctm); break;
> > > +			case 23: rc = ctf_member_info(cfp, type, "fsbase", &ctm); break;
> > > +			case 24: rc = ctf_member_info(cfp, type, "gsbase", &ctm); break;
> > > +			case 25: rc = ctf_member_info(cfp, type, "trap_nr", &ctm); break;
> > > +			}
> > > +			if (rc == -1)
> > > +				longjmp(yypcb->pcb_jmpbuf, EDT_NOCTF);
> > > +			offset += ctm.ctm_offset / NBBY;
> > > +
> > > +			/* copy task->thread.member onto the stack */
> > > +			if (dt_regset_xalloc_args(drp) == -1)
> > > +				longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > > +			dt_regset_xalloc(drp, BPF_REG_0);
> > > +			emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_task));
> > > +			emit(dlp, BPF_MOV_REG(BPF_REG_3, BPF_REG_0));
> > > +			emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, offset));
> > > +			emit(dlp, BPF_MOV_IMM(BPF_REG_2, sizeof(uint64_t)));
> > > +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_SP));
> > > +			emit(dlp, BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));
> > > +			dt_regset_free_args(drp);
> > > +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_FP, DT_STK_SP));
> > > +			emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_0, 0));
> > > +			dt_regset_free(drp, BPF_REG_0);
> > > +
> > > +			return;
> > > +		}
> > > +#endif
> > > +
> > > +		dnerror(dnp, D_UNKNOWN, "uregs[]: index out of bounds (%d)\n", idx);
> > > +
> > > +		return;
> > > +	}
> > > +
> > > +	/* if in-bounds, look up pt_regs[] */
> > > +	if (dtp->dt_bpfhelper[BPF_FUNC_get_current_task_btf] == BPF_FUNC_unspec
> > > +	  || dtp->dt_bpfhelper[BPF_FUNC_task_pt_regs] == BPF_FUNC_unspec)
> > > +		dnerror(dnp, D_UNKNOWN, "uregs[] is not supported on this kernel\n");
> > Can we move this to the beginning of the function, since really, if these
> > helpers do not exist, it is more consistent (as you mentioned before) to not
> > handle *any* indexing of uregs[].
> 
> As I mentioned before???  I think I was trying to say that the "consistent"
> error message did not make sense to me.  That is, if a user is interested in
> one of the task->thread. members, why should they be refused?  For reasons
> related to other, uninteresting (to them), uregs[]?  Users should not be
> refused features because unrelated features don't work.
> 
> Anyhow, you get the last word on this.  You can r-b or just let me know and
> I'll post a v6 with your proposed change.

I'd prefer to go with an all or nothing approach, especially because this is
documented for the user as an array and it would seem weird to me that you can
access some (rather obscure) values from the array but not the rest (which are
in fact the most obvious ones that you would expect to work).  The way this is
implemented, i.e. the way the values are retrieved from the kernel is not at
all exposed to the user and allowing partial retrieval (some values work but
most do not) is just confusing.

You can post a v6 or I can make the change as I add the patch to the branch -
either works for me.  It is just a code move.
> 
> > > +
> > > +	if (dt_regset_xalloc_args(drp) == -1)
> > > +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > > +	dt_regset_xalloc(drp, BPF_REG_0);
> > > +	emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_task_btf));
> > > +	emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_0));
> > > +	emit(dlp, BPF_CALL_HELPER(BPF_FUNC_task_pt_regs));
> > > +	dt_regset_free_args(drp);
> > > +	emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, idx * sizeof(uint64_t)));
> > > +	emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_0, 0));
> > > +	dt_regset_free(drp, BPF_REG_0);
> > > +}
> > > +
> > >   /*
> > >    * Currently, this code is only used for the args[] and uregs[] arrays.
> > >    */
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel