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

From: Oleg Nesterov
Date: Wed Jun 19 2024 - 10:41:10 EST


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@xxxxxxxxxx/
>
> Signed-off-by: Liao Chang <liaochang1@xxxxxxxxxx>
> ---
> 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
>