Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions arch/riscv/configs/qemu_riscv64cheripc_defconfig
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,7 @@ CONFIG_RCU_EQS_DEBUG=y
CONFIG_FUNCTION_TRACER=y
CONFIG_FUNCTION_GRAPH_RETVAL=y
CONFIG_FUNCTION_GRAPH_RETADDR=y
CONFIG_FPROBE=y
CONFIG_FUNCTION_PROFILER=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_TRACER_SNAPSHOT=y
Expand Down
1 change: 0 additions & 1 deletion arch/riscv/include/asm/Kbuild
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ syscall-y += syscall_table_64.h

generic-y += early_ioremap.h
generic-y += flat.h
generic-y += fprobe.h
generic-y += kvm_para.h
generic-y += mmzone.h
generic-y += mcs_spinlock.h
Expand Down
11 changes: 11 additions & 0 deletions arch/riscv/include/asm/fprobe.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef _ASM_RISCV_FPROBE_H
#define _ASM_RISCV_FPROBE_H

#include <asm-generic/fprobe.h>

#ifdef CONFIG_CHERI_KERNEL
#undef ARCH_DEFINE_ENCODE_FPROBE_HEADER
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to only set ARCH_DEFINE_ENCODE_FPROBE_HEADER for !CONFIG_CHERI_KERNEL independent of architecture as it shouldn't work for any architecture, right? I don't think this is specific to risc-v, it's just the first architecture with support for it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the other way around. Almost all archs can encode the fprobe header info into one unsigned long. Only risc-v cheri and loongarch can't do this.

#endif

#endif /* _ASM_RISCV_FPROBE_H */
4 changes: 2 additions & 2 deletions include/linux/fprobe.h
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ static inline void enable_fprobe(struct fprobe *fp)
fp->flags &= ~FPROBE_FL_DISABLED;
}

/* The entry data size is 4 bits (=16) * sizeof(long) in maximum */
/* The entry data size is 4 bits (=16) * <return stack's word size> in maximum */
#define FPROBE_DATA_SIZE_BITS 4
#define MAX_FPROBE_DATA_SIZE_WORD ((1L << FPROBE_DATA_SIZE_BITS) - 1)
#define MAX_FPROBE_DATA_SIZE (MAX_FPROBE_DATA_SIZE_WORD * sizeof(long))
#define MAX_FPROBE_DATA_SIZE (MAX_FPROBE_DATA_SIZE_WORD * RET_STACK_WORD_SIZE)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all replacements of sizeof(long) with RET_STACK_WORD_SIZE should be moved into the third patch where you introduce it. I think this would be cleaner as you are changing the used size in all of them and it also explains why RET_STACK_WORD_SIZE needs to be in the header and not .c files.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the shadow stack changes separate from the fprobe changes. The shadow stack is also used by the function graph tracer.


#endif
12 changes: 7 additions & 5 deletions include/linux/ftrace.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,14 +259,14 @@ static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
}

#ifdef CONFIG_HAVE_REGS_AND_STACK_ACCESS_API
static __always_inline unsigned long
static __always_inline uintptr_t
ftrace_regs_get_kernel_stack_nth(struct ftrace_regs *fregs, unsigned int nth)
{
unsigned long *stackp;
uintptr_t *stackp;

stackp = (unsigned long *)ftrace_regs_get_stack_pointer(fregs);
if (((unsigned long)(stackp + nth) & ~(THREAD_SIZE - 1)) ==
((unsigned long)stackp & ~(THREAD_SIZE - 1)))
stackp = (uintptr_t *)ftrace_regs_get_stack_pointer(fregs);
if (((uintptr_t)(stackp + nth) & ~(THREAD_SIZE - 1)) ==
((uintptr_t)stackp & ~(THREAD_SIZE - 1)))
return *(stackp + nth);

return 0;
Expand Down Expand Up @@ -1212,6 +1212,8 @@ struct ftrace_ret_stack {
uintptr_t *retp;
};

#define RET_STACK_WORD_SIZE sizeof(typeof(*current->ret_stack))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reduce this to sizeof(*current->ret_stack)?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, typeof() doesn't make things clearer.


/*
* Primary handler of a function return.
* It relays on ftrace_return_to_handler.
Expand Down
10 changes: 5 additions & 5 deletions kernel/trace/fgraph.c
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ enum {
#define FGRAPH_DATA_BITS 5
#define FGRAPH_DATA_MASK GENMASK(FGRAPH_DATA_BITS - 1, 0)
#define FGRAPH_DATA_SHIFT (FGRAPH_TYPE_SHIFT + FGRAPH_TYPE_BITS)
#define FGRAPH_MAX_DATA_SIZE (sizeof(long) * (1 << FGRAPH_DATA_BITS))
#define FGRAPH_MAX_DATA_SIZE (RET_STACK_WORD_SIZE * (1 << FGRAPH_DATA_BITS))

#define FGRAPH_DATA_INDEX_BITS 4
#define FGRAPH_DATA_INDEX_MASK GENMASK(FGRAPH_DATA_INDEX_BITS - 1, 0)
Expand Down Expand Up @@ -353,8 +353,8 @@ void *fgraph_reserve_data(int idx, int size_bytes)
if (size_bytes > FGRAPH_MAX_DATA_SIZE)
return NULL;

/* Convert the data size to number of longs. */
data_size = (size_bytes + sizeof(long) - 1) >> (sizeof(long) == 4 ? 2 : 3);
/* Convert the data size to number of stack words. */
data_size = DIV_ROUND_UP(size_bytes, RET_STACK_WORD_SIZE);

val = get_fgraph_entry(current, curr_ret_stack - 1);
data = &current->ret_stack[curr_ret_stack];
Expand Down Expand Up @@ -494,7 +494,7 @@ void *fgraph_retrieve_parent_data(int idx, int *size_bytes, int depth)
return NULL;
found:
if (size_bytes)
*size_bytes = __get_data_size(val) * sizeof(long);
*size_bytes = __get_data_size(val) * RET_STACK_WORD_SIZE;
return get_data_type_data(current, offset);
}

Expand Down Expand Up @@ -573,7 +573,7 @@ ftrace_push_return_trace(uintptr_t ret, unsigned long func,
if (!current->ret_stack)
return -EBUSY;

BUILD_BUG_ON(SHADOW_STACK_SIZE % sizeof(long));
BUILD_BUG_ON(SHADOW_STACK_SIZE % RET_STACK_WORD_SIZE);

/* Set val to "reserved" with the delta to the new fgraph frame */
val = (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_FRAME_OFFSET;
Expand Down
37 changes: 17 additions & 20 deletions kernel/trace/fprobe.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@

#define FPROBE_HASH_BITS 6
#define FPROBE_TABLE_SIZE (1 << FPROBE_HASH_BITS)

#define SIZE_IN_LONG(x) ((x + sizeof(long) - 1) >> (sizeof(long) == 8 ? 3 : 2))
#define SIZE_IN_LONG(x) DIV_ROUND_UP(x, RET_STACK_WORD_SIZE)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name SIZE_IN_LONG doesn't make sense any more. Maybe you could rename this to something more appropriate?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it's good to not diverge from upstream. But this is misleading, I'll rename it.


/*
* fprobe_table: hold 'fprobe_hlist::hlist' for checking the fprobe still
Expand Down Expand Up @@ -56,7 +55,7 @@ static struct fprobe_hlist_node *find_first_fprobe_node(unsigned long ip)
struct fprobe_hlist_node *node;
struct hlist_head *head;

head = &fprobe_ip_table[hash_ptr((void *)ip, FPROBE_IP_HASH_BITS)];
head = &fprobe_ip_table[hash_ptr(__c_fakep(ip), FPROBE_IP_HASH_BITS)];
hlist_for_each_entry_rcu(node, head, hlist,
lockdep_is_held(&fprobe_mutex)) {
if (node->addr == ip)
Expand All @@ -80,7 +79,7 @@ static void insert_fprobe_node(struct fprobe_hlist_node *node)
hlist_add_before_rcu(&node->hlist, &next->hlist);
return;
}
head = &fprobe_ip_table[hash_ptr((void *)ip, FPROBE_IP_HASH_BITS)];
head = &fprobe_ip_table[hash_ptr(__c_fakep(ip), FPROBE_IP_HASH_BITS)];
hlist_add_head_rcu(&node->hlist, head);
}

Expand Down Expand Up @@ -153,7 +152,7 @@ static int del_fprobe_hash(struct fprobe *fp)
/* The arch should encode fprobe_header info into one unsigned long */
#define FPROBE_HEADER_SIZE_IN_LONG 1

static inline bool write_fprobe_header(unsigned long *stack,
static inline bool write_fprobe_header(uintptr_t *stack,
struct fprobe *fp, unsigned int size_words)
{
if (WARN_ON_ONCE(size_words > MAX_FPROBE_DATA_SIZE_WORD ||
Expand All @@ -164,7 +163,7 @@ static inline bool write_fprobe_header(unsigned long *stack,
return true;
}

static inline void read_fprobe_header(unsigned long *stack,
static inline void read_fprobe_header(uintptr_t *stack,
struct fprobe **fp, unsigned int *size_words)
{
*fp = arch_decode_fprobe_header_fp(*stack);
Expand All @@ -177,11 +176,11 @@ static inline void read_fprobe_header(unsigned long *stack,
struct __fprobe_header {
struct fprobe *fp;
unsigned long size_words;
} __packed;
} __packed_if_not_cheri;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this __packed is actually needed or if it could be removed in general. Did you look into this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the __packed isn't really needed for non-cheri. Looking at the upstream code, the __packed was added when the second member was changed from int bits to unsigned long size_words. No idea why they did that.

__packed on cheri makes the compilation fail

kernel/trace/fprobe.c:176:8: error: alignment (1) of 'struct __fprobe_header' is less than the required capability alignment (16) [-Werror,-Wcheri-capability-misuse]
  176 | struct __fprobe_header {

The idea of __packed_if_not_cheri was to keep the non-cheri code unchanged.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this patch for removal in my list already, I will probably just propose the patch upstream and see if there are any arguments against removal.


#define FPROBE_HEADER_SIZE_IN_LONG SIZE_IN_LONG(sizeof(struct __fprobe_header))

static inline bool write_fprobe_header(unsigned long *stack,
static inline bool write_fprobe_header(uintptr_t *stack,
struct fprobe *fp, unsigned int size_words)
{
struct __fprobe_header *fph = (struct __fprobe_header *)stack;
Expand All @@ -194,7 +193,7 @@ static inline bool write_fprobe_header(unsigned long *stack,
return true;
}

static inline void read_fprobe_header(unsigned long *stack,
static inline void read_fprobe_header(uintptr_t *stack,
struct fprobe **fp, unsigned int *size_words)
{
struct __fprobe_header *fph = (struct __fprobe_header *)stack;
Expand All @@ -214,7 +213,7 @@ static inline void read_fprobe_header(unsigned long *stack,
* the shadow stack with its entry data size.
*
*/
static inline int __fprobe_handler(unsigned long ip, unsigned long parent_ip,
static inline int __fprobe_handler(unsigned long ip, uintptr_t parent_ip,
struct fprobe *fp, struct ftrace_regs *fregs,
void *data)
{
Expand All @@ -224,7 +223,7 @@ static inline int __fprobe_handler(unsigned long ip, unsigned long parent_ip,
return fp->entry_handler(fp, ip, parent_ip, fregs, data);
}

static inline int __fprobe_kprobe_handler(unsigned long ip, unsigned long parent_ip,
static inline int __fprobe_kprobe_handler(unsigned long ip, uintptr_t parent_ip,
struct fprobe *fp, struct ftrace_regs *fregs,
void *data)
{
Expand All @@ -250,9 +249,9 @@ static int fprobe_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
struct ftrace_regs *fregs)
{
struct fprobe_hlist_node *node, *first;
unsigned long *fgraph_data = NULL;
uintptr_t *fgraph_data = NULL;
unsigned long func = trace->func;
unsigned long ret_ip;
uintptr_t ret_ip;
int reserved_words;
struct fprobe *fp;
int used, ret;
Expand Down Expand Up @@ -280,7 +279,7 @@ static int fprobe_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
}
node = first;
if (reserved_words) {
fgraph_data = fgraph_reserve_data(gops->idx, reserved_words * sizeof(long));
fgraph_data = fgraph_reserve_data(gops->idx, reserved_words * RET_STACK_WORD_SIZE);
if (unlikely(!fgraph_data)) {
hlist_for_each_entry_from_rcu(node, hlist) {
if (node->addr != func)
Expand Down Expand Up @@ -328,8 +327,6 @@ static int fprobe_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
used += FPROBE_HEADER_SIZE_IN_LONG + size_words;
}
}
if (used < reserved_words)
memset(fgraph_data + used, 0, reserved_words - used);

/* If any exit_handler is set, data must be used. */
return used != 0;
Expand All @@ -340,13 +337,13 @@ static void fprobe_return(struct ftrace_graph_ret *trace,
struct fgraph_ops *gops,
struct ftrace_regs *fregs)
{
unsigned long *fgraph_data = NULL;
unsigned long ret_ip;
uintptr_t *fgraph_data = NULL;
uintptr_t ret_ip;
struct fprobe *fp;
int size, curr;
int size_words;

fgraph_data = (unsigned long *)fgraph_retrieve_data(gops->idx, &size);
fgraph_data = (uintptr_t *)fgraph_retrieve_data(gops->idx, &size);
if (WARN_ON_ONCE(!fgraph_data))
return;
size_words = SIZE_IN_LONG(size);
Expand Down Expand Up @@ -621,7 +618,7 @@ static int fprobe_init(struct fprobe *fp, unsigned long *addrs, int num)
if (!fp || !addrs || num <= 0)
return -EINVAL;

size = ALIGN(fp->entry_data_size, sizeof(long));
size = ALIGN(fp->entry_data_size, RET_STACK_WORD_SIZE);
if (size > MAX_FPROBE_DATA_SIZE)
return -E2BIG;
fp->entry_data_size = size;
Expand Down
6 changes: 3 additions & 3 deletions kernel/trace/trace.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,13 @@ struct kretprobe_trace_entry_head {

struct fentry_trace_entry_head {
struct trace_entry ent;
uintptr_t ip;
ptraddr_t ip;
};

struct fexit_trace_entry_head {
struct trace_entry ent;
uintptr_t func;
uintptr_t ret_ip;
ptraddr_t func;
ptraddr_t ret_ip;
};

#define TRACE_BUF_SIZE 1024
Expand Down
22 changes: 11 additions & 11 deletions kernel/trace/trace_fprobe.c
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ static unsigned long tracepoint_user_ip(struct tracepoint_user *tuser)
if (!tuser->tpoint)
return 0UL;

return (unsigned long)tuser->tpoint->probestub;
return __c_pa(tuser->tpoint->probestub);
}

static void __tracepoint_user_free(struct tracepoint_user *tuser)
Expand Down Expand Up @@ -279,7 +279,7 @@ process_fetch_insn(struct fetch_insn *code, void *rec, void *edata,
void *dest, void *base)
{
struct ftrace_regs *fregs = rec;
unsigned long val;
uintptr_t val;
int ret;

retry:
Expand All @@ -299,7 +299,7 @@ process_fetch_insn(struct fetch_insn *code, void *rec, void *edata,
val = ftrace_regs_get_argument(fregs, code->param);
break;
case FETCH_OP_EDATA:
val = *(unsigned long *)((unsigned long)edata + code->offset);
val = *(uintptr_t *)((uintptr_t)edata + code->offset);
break;
#endif
case FETCH_NOP_SYMBOL: /* Ignore a place holder */
Expand Down Expand Up @@ -363,7 +363,7 @@ static nokprobe_inline
void store_fprobe_entry_data(void *edata, struct trace_probe *tp, struct ftrace_regs *fregs)
{
struct probe_entry_arg *earg = tp->entry_arg;
unsigned long val = 0;
uintptr_t val = 0;
int i;

if (!earg)
Expand All @@ -377,7 +377,7 @@ void store_fprobe_entry_data(void *edata, struct trace_probe *tp, struct ftrace_
val = ftrace_regs_get_argument(fregs, code->param);
break;
case FETCH_OP_ST_EDATA:
*(unsigned long *)((unsigned long)edata + code->offset) = val;
*(uintptr_t *)((uintptr_t)edata + code->offset) = val;
break;
case FETCH_OP_END:
goto end;
Expand Down Expand Up @@ -429,15 +429,15 @@ __fexit_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
fbuffer.regs = ftrace_get_regs(fregs);
entry = fbuffer.entry = ring_buffer_event_data(fbuffer.event);
entry->func = entry_ip;
entry->ret_ip = ret_ip;
entry->ret_ip = __c_ua(ret_ip);
store_trace_args(&entry[1], &tf->tp, fregs, entry_data, sizeof(*entry), dsize);

trace_event_buffer_commit(&fbuffer);
}

static void
fexit_trace_func(struct trace_fprobe *tf, unsigned long entry_ip,
unsigned long ret_ip, struct ftrace_regs *fregs, void *entry_data)
uintptr_t ret_ip, struct ftrace_regs *fregs, void *entry_data)
{
struct event_file_link *link;

Expand Down Expand Up @@ -484,7 +484,7 @@ NOKPROBE_SYMBOL(fentry_perf_func);

static void
fexit_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
unsigned long ret_ip, struct ftrace_regs *fregs,
uintptr_t ret_ip, struct ftrace_regs *fregs,
void *entry_data)
{
struct trace_event_call *call = trace_probe_event_call(&tf->tp);
Expand All @@ -510,7 +510,7 @@ fexit_perf_func(struct trace_fprobe *tf, unsigned long entry_ip,
regs = ftrace_fill_perf_regs(fregs, regs);

entry->func = entry_ip;
entry->ret_ip = ret_ip;
entry->ret_ip = __c_ua(ret_ip);
store_trace_args(&entry[1], &tf->tp, fregs, entry_data, sizeof(*entry), dsize);
perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
head, NULL);
Expand Down Expand Up @@ -1318,7 +1318,7 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
sbuf = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
if (!sbuf)
return -ENOMEM;
ctx->funcname = kallsyms_lookup((unsigned long)tpoint->probestub,
ctx->funcname = kallsyms_lookup(__c_pa(tpoint->probestub),
NULL, NULL, NULL, sbuf);
}
}
Expand Down Expand Up @@ -1368,7 +1368,7 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
if (is_return && tf->tp.entry_arg) {
tf->fp.entry_handler = trace_fprobe_entry_handler;
tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);
if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
if (ALIGN(tf->fp.entry_data_size, RET_STACK_WORD_SIZE) > MAX_FPROBE_DATA_SIZE) {
trace_probe_log_set_index(2);
trace_probe_log_err(0, TOO_MANY_EARGS);
return -E2BIG;
Expand Down
Loading