Re: [PATCH v4 1/2] riscv: probes: reject kprobes inside LR/SC sequences

[Nam Cao <namcao@linutronix.de>]

Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com> writes:
> kallsyms_lookup_size_offset() returns the offset of the probe from the
> start of the enclosing symbol, which is used to find the function start.
> If the probe address exactly matched a kallsyms symbol, that offset would
> be 0 and the forward scan would be skipped, silently missing the LR/SC
> sequence.

...okay, that sounds like a problem.

> To avoid this, global labels should not be placed at interior
> instructions of an LR/SC sequence.

"Should not be placed" by whom? Kernel developers, compilers? And how do
we ensure that?

> In practice this is not a
> restriction: LR/SC sequences are tight retry loops and generally do not
> carry global labels inside them, so the enclosing function start is
> resolved correctly and the forward scan proceeds as intended.

"Generally" not a problem is not good enough, sorry. It either is a
problem and we must deal with it, or it is never a problem.

I am not sure if labels will be placed between LR and SC. But a label on
the LR instruction sounds entirely possible.

What confuses me is that this case does not sound difficult to
accommodate. Any reason why we should not or cannot do that?

> +/*
> + * A trap taken in the middle of an LR/SC sequence clears the load
> + * reservation, so an SC following the probed instruction would always
> + * fail and the enclosing retry loop would re-enter the breakpoint.
> + * Reject probes inside such a sequence.
> + *
> + * A constrained LR/SC loop (Zalrsc) is at most 16 instructions and
> + * must be contained in a 64-byte contiguous region of memory, so only
> + * instructions within that window preceding the probe can open a
> + * sequence containing it.  RISC-V instruction boundaries cannot be
> + * recovered by walking backwards - a 32-bit instruction whose upper
> + * halfword looks like a compressed instruction is ambiguous - so walk
> + * forward from the function start, a known instruction boundary, up to
> + * the probe address and track whether an LR is still outstanding.
> + */
> +#define MAX_ATOMIC_CONTEXT_SIZE	64

Doesn't this mean we can walk for more than 16 instructions and
therefore can have false rejection?

> +static bool __kprobes riscv_probe_insn_in_atomic(unsigned long addr)
> +{
> +	unsigned long start, offset, pc;
> +	bool in_atomic = false;
> +
> +	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
> +		return false;
> +
> +	start = addr - offset;
> +	pc = start;
> +
> +	while (pc < addr) {
> +		u16 halfword = *(u16 *)pc;
> +		unsigned int len = (halfword & 0x3) == 0x3 ? 4 : 2;

We already have a macro to get the instruction length, please use that.

And why do we need to separately load 16-bit 'halfword' here, and load
32-bit 'insn' later on? Can't we use 'insn' for both usage?

> +
> +		if (addr - pc <= MAX_ATOMIC_CONTEXT_SIZE) {
> +			if (len == 4) {
> +				u32 insn = get_unaligned((u32 *)pc);
> +
> +				if (riscv_insn_is_lr(insn))
> +					in_atomic = true;
> +				else if (riscv_insn_is_sc(insn))
> +					in_atomic = false;
> +			}
> +		}
> +
> +		pc += len;
> +	}
> +
> +	return in_atomic;
> +}
> +
>  int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  {
>  	u16 *insn = (u16 *)p->addr;
> @@ -79,6 +130,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  	if (!arch_check_kprobe((unsigned long)p->addr))
>  		return -EILSEQ;
>  
> +	if (riscv_probe_insn_in_atomic((unsigned long)p->addr))
> +		return -EINVAL;
> +

arch_check_kprobe() is already doing a walk. It probably is a good idea
to merge this with that, to avoid walking twice.

Nam

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv