Re: [PATCH bpf-next] uprobes: Fix the xol slots reserved for uretprobe trampoline

[Oleg Nesterov <oleg@redhat.com>]

On 06/19, Liao Chang wrote:
>
> When the new uretprobe system call was added [1], the xol slots reserved
> for the uretprobe trampoline might be insufficient on some architecture.

Confused... this doesn't depend on the change above?

> For example, on arm64, the trampoline is consist of three instructions
> at least. So it should mark enough bits in area->bitmaps and
> and area->slot_count for the reserved slots.

Do you mean that on arm64 UPROBE_SWBP_INSN_SIZE > UPROBE_XOL_SLOT_BYTES ?

From arch/arm64/include/asm/uprobes.h

	#define MAX_UINSN_BYTES		AARCH64_INSN_SIZE

	#define UPROBE_SWBP_INSN	cpu_to_le32(BRK64_OPCODE_UPROBES)
	#define UPROBE_SWBP_INSN_SIZE	AARCH64_INSN_SIZE
	#define UPROBE_XOL_SLOT_BYTES	MAX_UINSN_BYTES

	typedef __le32 uprobe_opcode_t;

	struct arch_uprobe_task {
	};

	struct arch_uprobe {
		union {
			u8 insn[MAX_UINSN_BYTES];
			u8 ixol[MAX_UINSN_BYTES];

So it seems that UPROBE_SWBP_INSN_SIZE == MAX_UINSN_BYTES and it must
be less than UPROBE_XOL_SLOT_BYTES, otherwise

arch_uprobe_copy_ixol(..., uprobe->arch.ixol, sizeof(uprobe->arch.ixol))
in xol_get_insn_slot() won't fit the slot as well?

OTOH, it look as if UPROBE_SWBP_INSN_SIZE == UPROBE_XOL_SLOT_BYTES, so
I don't understand the problem...

Oleg.

> [1] https://lore.kernel.org/all/20240611112158.40795-4-jolsa@kernel.org/
> 
> Signed-off-by: Liao Chang <liaochang1@huawei.com>
> ---
>  kernel/events/uprobes.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 2816e65729ac..efd2d7f56622 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1485,7 +1485,7 @@ void * __weak arch_uprobe_trampoline(unsigned long *psize)
>  static struct xol_area *__create_xol_area(unsigned long vaddr)
>  {
>  	struct mm_struct *mm = current->mm;
> -	unsigned long insns_size;
> +	unsigned long insns_size, slot_nr;
>  	struct xol_area *area;
>  	void *insns;
>  
> @@ -1508,10 +1508,13 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
>  
>  	area->vaddr = vaddr;
>  	init_waitqueue_head(&area->wq);
> -	/* Reserve the 1st slot for get_trampoline_vaddr() */
> -	set_bit(0, area->bitmap);
> -	atomic_set(&area->slot_count, 1);
>  	insns = arch_uprobe_trampoline(&insns_size);
> +	/* Reserve enough slots for the uretprobe trampoline */
> +	for (slot_nr = 0;
> +	     slot_nr < max((insns_size / UPROBE_XOL_SLOT_BYTES), 1);
> +	     slot_nr++)
> +		set_bit(slot_nr, area->bitmap);
> +	atomic_set(&area->slot_count, slot_nr);
>  	arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size);
>  
>  	if (!xol_add_vma(mm, area))
> -- 
> 2.34.1
>