NACK: [SRU][jammy][PATCH v2 3/5] kprobes: Add kretprobe_find_ret_addr() for searching return address
John Cabaj
john.cabaj at canonical.com
Thu Apr 6 15:15:36 UTC 2023
On 4/6/23 8:42 AM, Stefan Bader wrote:
> On 31.03.23 16:00, John Cabaj wrote:
>> From: Masami Hiramatsu <mhiramat at kernel.org>
>>
>> BugLink: https://bugs.launchpad.net/bugs/2013603 (Kernel livepatch ftrace graph fix)
>>
>> Introduce kretprobe_find_ret_addr() and is_kretprobe_trampoline().
>> These APIs will be used by the ORC stack unwinder and ftrace, so that
>> they can check whether the given address points kretprobe trampoline
>> code and query the correct return address in that case.
>>
>> Link: https://lkml.kernel.org/r/163163046461.489837.1044778356430293962.stgit@devnote2
>>
>> Signed-off-by: Masami Hiramatsu <mhiramat at kernel.org>
>> Tested-by: Andrii Nakryiko <andrii at kernel.org>
>> Signed-off-by: Steven Rostedt (VMware) <rostedt at goodmis.org>
>> (backported from commit 03bac0df2886882c43e6d0bfff9dee84a184fc7e)
>> [John Cabaj: cleaning-up kernel panic that was relocated to conditional]
>> Signed-off-by: John Cabaj <john.cabaj at canonical.com>
>> ---
>> include/linux/kprobes.h | 22 ++++++++
>> kernel/kprobes.c | 109 ++++++++++++++++++++++++++++++----------
>> 2 files changed, 105 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>> index f6b0aef35f66..9e24d8b79100 100644
>> --- a/include/linux/kprobes.h
>> +++ b/include/linux/kprobes.h
>> @@ -511,6 +511,28 @@ static inline bool is_kprobe_optinsn_slot(unsigned long addr)
>> }
>> #endif
>> +#ifdef CONFIG_KRETPROBES
>> +static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
>> +{
>> + return (void *)addr == kretprobe_trampoline_addr();
>> +}
>> +
>> +unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
>> + struct llist_node **cur);
>> +#else
>> +static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
>> +{
>> + return false;
>> +}
>> +
>> +static nokprobe_inline
>> +unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
>> + struct llist_node **cur)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> /* Returns true if kprobes handled the fault */
>> static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs,
>> unsigned int trap)
>
> The set fails to apply to current jammy:linux/master-next:
>
> Applying: kprobes: treewide: Remove trampoline_address from kretprobe_trampoline_handler()
> Applying: kprobes: treewide: Make it harder to refer kretprobe_trampoline directly
> Applying: kprobes: Add kretprobe_find_ret_addr() for searching return address
> error: patch failed: kernel/kprobes.c:1866
> error: kernel/kprobes.c: patch does not apply
> Patch failed at 0003 kprobes: Add kretprobe_find_ret_addr() for searching return address
>
> checking file include/linux/kprobes.h
> checking file kernel/kprobes.c
> Hunk #1 FAILED at 1866.
> Hunk #2 succeeded at 1913 (offset -2 lines).
> 1 out of 2 hunks FAILED
This likely collided with changes being applied since this patch was originally submitted, possibly c3694073333c ("kprobes: treewide: Cleanup the error messages for kprobes")
Resubmitted and checked with a re-application - hopefully it doesn't conflict with more kprobes changes before next application.
Thanks,
John
>
> Please submit again (v3) once this is fixed.
>
> -Stefan
>
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index 7da4a5048eb7..8e06a8d6516c 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
>> @@ -1866,45 +1866,87 @@ unsigned long __weak arch_deref_entry_point(void *entry)
>> #ifdef CONFIG_KRETPROBES
>> -unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
>> - void *frame_pointer)
>> +/* This assumes the 'tsk' is the current task or the is not running. */
>> +static kprobe_opcode_t *__kretprobe_find_ret_addr(struct task_struct *tsk,
>> + struct llist_node **cur)
>> {
>> - kprobe_opcode_t *correct_ret_addr = NULL;
>> struct kretprobe_instance *ri = NULL;
>> - struct llist_node *first, *node;
>> - struct kretprobe *rp;
>> + struct llist_node *node = *cur;
>> +
>> + if (!node)
>> + node = tsk->kretprobe_instances.first;
>> + else
>> + node = node->next;
>> - /* Find all nodes for this frame. */
>> - first = node = current->kretprobe_instances.first;
>> while (node) {
>> ri = container_of(node, struct kretprobe_instance, llist);
>> -
>> - BUG_ON(ri->fp != frame_pointer);
>> -
>> if (ri->ret_addr != kretprobe_trampoline_addr()) {
>> - correct_ret_addr = ri->ret_addr;
>> - /*
>> - * This is the real return address. Any other
>> - * instances associated with this task are for
>> - * other calls deeper on the call stack
>> - */
>> - goto found;
>> + *cur = node;
>> + return ri->ret_addr;
>> }
>> -
>> node = node->next;
>> }
>> - pr_err("Oops! Kretprobe fails to find correct return address.\n");
>> - BUG_ON(1);
>> + return NULL;
>> +}
>> +NOKPROBE_SYMBOL(__kretprobe_find_ret_addr);
>> -found:
>> - /* Unlink all nodes for this frame. */
>> - current->kretprobe_instances.first = node->next;
>> - node->next = NULL;
>> +/**
>> + * kretprobe_find_ret_addr -- Find correct return address modified by kretprobe
>> + * @tsk: Target task
>> + * @fp: A frame pointer
>> + * @cur: a storage of the loop cursor llist_node pointer for next call
>> + *
>> + * Find the correct return address modified by a kretprobe on @tsk in unsigned
>> + * long type. If it finds the return address, this returns that address value,
>> + * or this returns 0.
>> + * The @tsk must be 'current' or a task which is not running. @fp is a hint
>> + * to get the currect return address - which is compared with the
>> + * kretprobe_instance::fp field. The @cur is a loop cursor for searching the
>> + * kretprobe return addresses on the @tsk. The '*@cur' should be NULL at the
>> + * first call, but '@cur' itself must NOT NULL.
>> + */
>> +unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
>> + struct llist_node **cur)
>> +{
>> + struct kretprobe_instance *ri = NULL;
>> + kprobe_opcode_t *ret;
>> +
>> + if (WARN_ON_ONCE(!cur))
>> + return 0;
>> +
>> + do {
>> + ret = __kretprobe_find_ret_addr(tsk, cur);
>> + if (!ret)
>> + break;
>> + ri = container_of(*cur, struct kretprobe_instance, llist);
>> + } while (ri->fp != fp);
>> - /* Run them.. */
>> + return (unsigned long)ret;
>> +}
>> +NOKPROBE_SYMBOL(kretprobe_find_ret_addr);
>> +
>> +unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
>> + void *frame_pointer)
>> +{
>> + kprobe_opcode_t *correct_ret_addr = NULL;
>> + struct kretprobe_instance *ri = NULL;
>> + struct llist_node *first, *node = NULL;
>> + struct kretprobe *rp;
>> +
>> + /* Find correct address and all nodes for this frame. */
>> + correct_ret_addr = __kretprobe_find_ret_addr(current, &node);
>> + if (!correct_ret_addr) {
>> + pr_err("kretprobe: Return address not found, not execute handler. Maybe there is a bug in the kernel.\n");
>> + BUG_ON(1);
>> + }
>> +
>> + /* Run the user handler of the nodes. */
>> + first = current->kretprobe_instances.first;
>> while (first) {
>> ri = container_of(first, struct kretprobe_instance, llist);
>> - first = first->next;
>> +
>> + if (WARN_ON_ONCE(ri->fp != frame_pointer))
>> + break;
>> rp = get_kretprobe(ri);
>> if (rp && rp->handler) {
>> @@ -1915,6 +1957,21 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
>> rp->handler(ri, regs);
>> __this_cpu_write(current_kprobe, prev);
>> }
>> + if (first == node)
>> + break;
>> +
>> + first = first->next;
>> + }
>> +
>> + /* Unlink all nodes for this frame. */
>> + first = current->kretprobe_instances.first;
>> + current->kretprobe_instances.first = node->next;
>> + node->next = NULL;
>> +
>> + /* Recycle free instances. */
>> + while (first) {
>> + ri = container_of(first, struct kretprobe_instance, llist);
>> + first = first->next;
>> recycle_rp_inst(ri);
>> }
>
More information about the kernel-team
mailing list