[DTrace-devel] [PATCH 01/12] Support loading scalars from kernel addresses
Kris Van Hees
kris.van.hees at oracle.com
Tue Jul 26 22:29:24 UTC 2022
On Tue, Jul 26, 2022 at 01:34:11PM -0700, Eugene Loh wrote:
> On 7/25/22 15:53, Kris Van Hees wrote:
>
> > On Thu, Jul 21, 2022 at 08:24:48PM -0700, Eugene Loh via DTrace-devel wrote:
> > > Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> > >
> > > For other new tests, other sanity checks? E.g., that max_pfn is a multiple
> > > of 4096? More stringent tests would be nice.
> > No, because that is not relevant to DTrace. Yes, it should be, but that is an
> > aspect of what that variable means at the kernel level.
>
> The point is not to check the kernel or a specific variable. The point
> would be to design a test where we check the test's (that is, DTrace's)
> output. If max_pfn is not suitable for that purpose, some other test should
> be designed.
The purpose of the test is to determine whether we can read a scalar from a
kernel address. Incidentally, I have added one that is verifying a very
predictable value along with the sign extension for < 64-bit values. So that
is covered already anyway now.
> >
> > > A few other things.
> > >
> > > On 7/13/22 12:17, Kris Van Hees via DTrace-devel wrote:
> > > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > >
> > > > + dt_regset_t *drp)
> > > > +{
> > > > + if (dt_regset_xalloc_args(drp) == -1)
> > > > + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> > > > + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_SP));
> > > > + emit(dlp, BPF_MOV_IMM(BPF_REG_2, size));
> > > > + emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> > > It is conceivable that dnp->dn_reg is either %r1 or %r2, in which case we
> > > just finished overwriting that register. Just make the %r3=%dnp->dn->reg
> > > instruction the first of the three.
> > This is the exiting issue with (lack of) proper register spilling. In general,
> > our code generation fails miserably whenever we start using %r1-%r5 and yet
> > have to call a function (incl. helpers). So while theoretically, the scenario
> > you suggest could happen, it generally means we're already going to get into
> > trouble.
>
> The fix for this specific problem is simple. The fix for the general
> problem is not (so far as I know) scheduled. It would seem to me to be best
> to deal with this specific problem now.
THe fix for the general problem has been in the works for a while now, even
though it is not finished. So it is certainly not some 'who knows when' item
on the todo list.
Anyway, I will fix it as you suggest - do you happen to have a test that shows
this problem occuring? We should have one if it is a realistic issue, and then
we can be sure that accidental changes to this code do not reintroduce the
problem you describe.
> > Also, if dnp->dn_reg is %r1 ir %r2, then it is very likely some other
> > value is stored in %r3 (since we allocate registers in descending order) and
> > will get clobbered.
>
> I do not get this point. If some other value is stored in %r3, it will be
> spilled/filled.
>
> >
> > > > + emit(dlp, BPF_MOV_REG(dnp->dn_reg, BPF_REG_1));
> > > It is conceivable that dnp->dn_reg is in the range %r1-%r5. If that's the
> > > case, then free_args() will fill the register with an old value and the
> > > stack pointer will have been lost. Might as well just omit this instruction
> > > and regenerate the stack pointer when you need it.
> > Same as above - I'd rather not optimmize based on the (broken) spill/fill
> > code.
>
> Yes, "same as above." The solution to the immediate problem is simple and
> the solution to the general problem is (IIUC) nowhere in sight?
More information about the DTrace-devel
mailing list