[DTrace-devel] [PATCH 01/12] Support loading scalars from kernel addresses

[Eugene Loh]

On 7/26/22 15:29, Kris Van Hees wrote:

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

Terrific.

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

Also terrific.  Good to know.

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

  Yeah.  Good idea.  How about:

# dtrace -qn '
BEGIN
{
   a = 1;
   b = 2;
   c = 4;
   d = 8;
   e = 16;
   f = 32;
   g = 64;
   h = 128;
   i = 256;

   /* each line is annotated with the register used for the kernel 
scalar load */
   printf("%x\n", `max_pfn        ); /* %r8 */
   printf("%x\n",   a + `max_pfn        ); /* %r7 */
   printf("%x\n",   a + (b + `max_pfn       )); /* %r6 */
   printf("%x\n",   a + (b + (c + `max_pfn      ))); /* %r5 */
   printf("%x\n",   a + (b + (c + (d + `max_pfn     )))); /* %r4 */
   printf("%x\n",   a + (b + (c + (d + (e + `max_pfn    ))))); /* %r3 */
   printf("%x\n",   a + (b + (c + (d + (e + (f + `max_pfn   )))))); /* 
%r2 */
   printf("%x\n",   a + (b + (c + (d + (e + (f + (g + `max_pfn  ))))))); 
/* %r1 */
   printf("%x\n",   a + (b + (c + (d + (e + (f + (g + (h + `max_pfn 
)))))))); /* %r8 */
   printf("%x\n",   a + (b + (c + (d + (e + (f + (g + (h + (i + 
`max_pfn))))))))); /* %r7 */
   exit(0);
}'

210000
210001
210003
210007
21000f
21001f
21003f
21007f
2100ff
2101ff

Without the fix, as soon as you get to the %r5 line, the program no 
longer loads due to verifier problems.  With the fixes, the register 
allocation even wraps all the way around!