[DTrace-devel] [PATCH] cg: use indirect load instructions for scalar DPTR and ALLOCA pointers

[Kris Van Hees]

On Mon, Feb 20, 2023 at 04:45:58PM +0000, Nick Alcock wrote:
> On 20 Feb 2023, Kris Van Hees via DTrace-devel uttered the following:
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> 
> I guess this is an efficiency improvement, not a correctness fix,
> because there's no need to mess around doing probe_reads on something we
> know is perfectly accessible (and the verifier should know it too).

Correct.

> > ---
> >  libdtrace/dt_cg.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index cc6ca05c..38c43df3 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> (DT_TOK_DEREF)
> > @@ -5395,7 +5395,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >  						 dnp->dn_reg);
> >  			}
> >  
> > -			if (dnp->dn_child->dn_flags & DT_NF_DPTR)
> > +			if (dnp->dn_child->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
> >  				emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
> >  			else
> >  				dt_cg_load_scalar(dnp, op, size, dlp, drp);
> 
> ode right above here is a dt_cg_alloca_access_check and a
> dt_cg_alloca_ptr to turn the alloca pointer into a real dereferenceable
> pointer...

Yes.  For alloca we need a real poiter, for dptr it already is.  And in both
cases we can use an indirect load.

> (DT_TOK_DOT)
> > @@ -5546,7 +5546,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >  
> >  			op = dt_cg_ldsize(dnp, ctfp, m.ctm_type, &size);
> >  
> > -			if (dnp->dn_left->dn_flags & DT_NF_DPTR)
> > +			if (dnp->dn_left->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
> >  				emit(dlp, BPF_LOAD(op, dnp->dn_left->dn_reg, dnp->dn_left->dn_reg, 0));
> >  			else
> >  				dt_cg_load_scalar(dnp->dn_left, op, size, dlp, drp);
> 
> ... but this doesn't have either of those, not even indirectly, because
> it must already have been dereffed. I'd think no alloca-marked pointer
> should be able to propagate this far: the whole point of an alloca
> marking is that it needs an access check and ptr before it can be used,
> which happens at dereference time. (And indeed we do not transfer alloca
> taint past DT_TOK_DOT.)
> 
> Why do you think this hunk is necessary? (What am I missing? Is this for
> structures in alloca space? If so, we need to spot that case and do a
> dt_cg_alloca_access_check and dt_cg_alloca_ptr here too. We still don't
> need to propagate taint past DT_TOK_DOT because checking the taintedness
> of the LHS of the dot operator will suffice: by the time we process the
> RHS, a few lines lower, we will have done the necessary access_check/ptr
> dance, so the RHS and points beyond it should not be alloca-tainted,
> just as DT_TOK_DEREF doesn't propagate alloca taint.)

You are correct that we currently cannot have an ALLOCA pointer here *but*
that is because there seems to be a propagation that is missed when we use
something like:

struct outer {
        struct inner {
                int     in;
        }       out;
} *p;

BEGIN {
        p = alloca(sizeof(struct outer));
	trace(p->out.in);
        exit(0);
}

In this case, we lose the ALLOCA marker once p->out is handled by the code
generator, and .in is then applied to an arbitrary pointer value rather than
a known entity (ALLOCA pointer).  So for now, the code always defaults to
using the dt_cg_load_scalar() mechanism.

I'll add to the todo list that we need to sort out this ALLOCA propagation in
this case, which will then ensure that an indirect load is generated for this
expression (p->out.in).

So, I am going to leave this chunk in because it should be needed :)

r-b ?