[DTrace-devel] [PATCH v2 03/17] cg: fault improvements

[Nick Alcock]

On 24 Mar 2022, Eugene Loh via DTrace-devel verbalised:

> On 3/14/22 5:30 PM, Nick Alcock wrote:
>
>> Add a label to dt_cg_probe_error, which is dropped on the first
>
> Dropped?  That sounds like "omitted".  How about "attached to"?

Much better! Adjusted.

> But what is even the point of this?  Most of the time, the label is not needed.  For the few times you want it, just insert
>         emitl(dlp, MYLABEL,
>                    BPF_NOP());
> in front of the dt_cg_probe_error() call.  The NOP will be optimized away anyhow and you'll get the same BPF instructions.

Errr... the NOP ends up being an actual instruction in the stream. I
thought we were at least trying to minimize our insn count, at least for
frequently-used things like the bounds checkers, where the complexity
cost of doing so is low?

>> instruction if set; eschew regset allocation on the grounds that
>
> If set?  According to this patch, the label is always set.  I'd omit
> "if set".  (Though mostly, as I said, I'd omit the label stuff
> altogether.)

DT_LBL_NONE is "unset", isn't it?

> I suppose "eschew" is fine, but "do not use" is simpler.

True! Adjusted.

>> spilling to the stack is pointless when the regs will all be thrown away
>> in any case, and can trigger errors if some of the regs are not properly
>> initialized, as is relatively likely in an error condition.
>
> Good point.  Indeed, I think you can be more emphatic:
> s/can trigger errors/will trigger BPF verifier errors/
> s/ properly//

Adjusted.

>> Also add a new dt_cg_probe_error_regval, which is like dt_cg_probe_error
>> except the last argument is not an illegal value but a register whose
>> value should be emitted.
>
> Actually, I like what you did with dt_cg_trace().  How about just
> adding an isreg flag to dt_cg_probe_error() since the differences
> between the existing and new functions are otherwise minimal.

Back when I wrote this dt_cg_probe_error had so many args I couldn't
remember which was which as it was. It has fewer now, thanks to Kris,
and this actually lets us simplify a lot of uses of regval, like this:

        if (sizemax < 0)
                dt_cg_probe_error(yypcb, lbl_size_err, DTRACEFLT_BADSIZE, size);
        else
                dt_cg_probe_error_regval(yypcb, lbl_size_err, DTRACEFLT_BADSIZE,
                                         size);

which can now become a simple

        dt_cg_probe_error(yypcb, lbl_size_erro, DTRACEFLT_BADSIZE,
                          (sizemax >= 0), size);

("if sizemax >= 0, this is a reg").

Adjusted!

>> This is a bit trickier: we must move the
>> illegal register arg into place first because we don't use the regset
>> allocation code, so it might be using one of the regs we're about to
>> overwrite; and don't emit a move at all if we're by chance already using
>> that reg (the verifier doesn't like mov %r4, %r4 very much).
>
> I'd omit the stuff from "This is a bit tricker" until the end of the
> paragraph.  It seems premature to get into that detail here.  The
> comments in the code suffice.

(Done.)

> Incidentally, are you sure "mov %r4 %r4" is problematic?  If %r4 is
> readable, this should be just fine.  And someday I can imagine we'll
> do code optimization that removes such no-ops.

I wish, but nope! It's broken for me, at least with some kernels (5.15
was the one I saw it on). It might be specific to the
using-non-allocated-register thing though, which is pretty specific to
faulting paths. (Though I didn't think allocating registers did anything
visible at the BPF layer, so probably not.)

> Also, the issue is not that regset_alloc() is not being used.  Even if
> you used regset_alloc_args(), you'd still want to write %r4 first.

True enough!

>>   @@ -666,16 +669,58 @@ dt_cg_probe_error(dt_pcb_t *pcb, uint32_t off, uint32_t fault, uint64_t illval)
>>   	 *				// call dt_probe_error
>>   	 *	return;			// exit
>>   	 */
>
> Incidentally, the comment block (not completely visible here) has DT_STK_DCT.  That's a pre-existing problem but might as well clean
> up that typo while you're in here.

(Fixed.)

>> -	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_DCTX));
>> +
>> +	emitl(dlp, lbl,
>> +		   BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
>>   	emit(dlp,  BPF_MOV_IMM(BPF_REG_2, off));
>>   	emit(dlp,  BPF_MOV_IMM(BPF_REG_3, fault));
>>   	emit(dlp,  BPF_MOV_IMM(BPF_REG_4, illval));
>> -	dt_regset_xalloc(drp, BPF_REG_0);
>>   	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
>> -	dt_regset_free_args(drp);
>> -	dt_regset_free(drp, BPF_REG_0);
>> +	emit(dlp,  BPF_RETURN());
>> +}
>> +
>> +/*
>> + * Generate code for a fault condition with the illval coming from a register.
>> + * Drop a label at the start.
>
> Again, I advise against this label stuff, but regardless "drop" sounds like "omit".

Yep. Changed to "... possibly at a given label". Keeping the label stuff
simply because it saves several instructions per bounds check and the
bounds checks are too damn big already and can't be moved out of line :(

>> + * Doesn't use the regset code on the grounds that when this is executed we will
>> + * only reuse one caller's reg in a stereotyped manner.
>
> Well, or that we are about to exit the function and so there is no point in spilling register contents to the stack.

Changed to

 * Doesn't use the regset code on the grounds that when this is executed we will
 * never reuse any of the callers' regs in any case.

(some time ago.)

>> + */
>> +static void
>> +dt_cg_probe_error_regval(dt_pcb_t *pcb, int lbl, uint32_t off,
>> +			 uint32_t fault, int illreg)
>> +{
>> +	dt_irlist_t	*dlp = &pcb->pcb_ir;
>> +	dt_ident_t	*idp = dt_dlib_get_func(yypcb->pcb_hdl,
>> +						"dt_probe_error");
>> +
>> +	assert(idp != NULL);
>> +
>> +	/*
>> +	 *	dt_probe_error(
>> +	 *		dctx,		// lddw %r1, %fp, DT_STK_DCTX
>> +	 *		off,		// mov %r2, off
>> +	 *		fault,		// mov %r3, fault
>> +	 *		illreg);	// mov %r4, %illreg
>
> I'm not a fan of these comments with disassembly lines but in any case here the disassembly instructions in the comment are in a
> different order from what one actually does.

Yeah, I've stopped using them too now I can read this stuff better. This
whole function is gone now in favour of an ILLISREG arg to
dt_cg_probe_error anyway. :)

-- 
NULL && (void)