Hi,
Recently while staring at kprobes code for an unrelated task, I
noticed some opportunities for improving the code.
The first patch fixes a long standing bug in kprobes debug
functionality. The remaining patches (marked RFC) refactor the code to
increase code sharing, improve readability and remove redundancy.
All feedback welcome.
Thanks,
Punit
Punit Agrawal (5):
kprobes: Do not use local variable when creating debugfs file
kprobes: Use helper to parse boolean input from userspace
kprobe: Simplify prepare_kprobe() by dropping redundant version
csky: ftrace: Drop duplicate implementation of
arch_check_ftrace_location()
kprobes: Make arch_check_ftrace_location static
arch/csky/kernel/probes/ftrace.c | 7 ----
include/linux/kprobes.h | 7 ++--
kernel/kprobes.c | 58 ++++++++++----------------------
3 files changed, 23 insertions(+), 49 deletions(-)
--
2.30.2
debugfs_create_file() takes a pointer argument that can be used during
file operation callbacks (accessible via i_private in the inode
structure). An obvious requirement is for the pointer to refer to
valid memory when used.
When creating the debugfs file to dynamically enable / disable
kprobes, a pointer to local variable is passed to
debugfs_create_file(); which will go out of scope when the init
function returns. The reason this hasn't triggered random memory
corruption is because the pointer is not accessed during the debugfs
file callbacks.
Fix the incorrect (and unnecessary) usage of local variable during
debugfs_file_create() by passing NULL instead.
Signed-off-by: Punit Agrawal <[email protected]>
---
kernel/kprobes.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 745f08fdd7a6..fdb1ea2e963b 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2816,13 +2816,12 @@ static const struct file_operations fops_kp = {
static int __init debugfs_kprobe_init(void)
{
struct dentry *dir;
- unsigned int value = 1;
dir = debugfs_create_dir("kprobes", NULL);
debugfs_create_file("list", 0400, dir, NULL, &kprobes_fops);
- debugfs_create_file("enabled", 0600, dir, &value, &fops_kp);
+ debugfs_create_file("enabled", 0600, dir, NULL, &fops_kp);
debugfs_create_file("blacklist", 0400, dir, NULL,
&kprobe_blacklist_fops);
--
2.30.2
arch_check_ftrace_location() was introduced as a weak function in
f7f242ff0044 ("kprobes: introduce weak arch_check_ftrace_location()
helper function") to allow architectures to handle kprobes call site
on their own.
Recently, the only architecture (csky) to implement
arch_check_ftrace_location() was migrated to using the common
version.
As a result, further cleanup the code to drop the weak attribute and
rename the function to remove the architecture specific
implementation.
Signed-off-by: Punit Agrawal <[email protected]>
---
include/linux/kprobes.h | 2 --
kernel/kprobes.c | 4 ++--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 771013bab18a..beea9ecee187 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -369,8 +369,6 @@ static inline int arch_prepare_kprobe_ftrace(struct kprobe *p)
}
#endif
-int arch_check_ftrace_location(struct kprobe *p);
-
/* Get the kprobe at this addr (if any) - called with preemption disabled */
struct kprobe *get_kprobe(void *addr);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 54d37d4ab897..b12ae6cc8dc3 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1531,7 +1531,7 @@ static inline int warn_kprobe_rereg(struct kprobe *p)
return ret;
}
-int __weak arch_check_ftrace_location(struct kprobe *p)
+static int check_ftrace_location(struct kprobe *p)
{
unsigned long ftrace_addr;
@@ -1554,7 +1554,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
{
int ret;
- ret = arch_check_ftrace_location(p);
+ ret = check_ftrace_location(p);
if (ret)
return ret;
jump_label_lock();
--
2.30.2
On Wed, 9 Jun 2021 19:50:19 +0900
Punit Agrawal <[email protected]> wrote:
> arch_check_ftrace_location() was introduced as a weak function in
> f7f242ff0044 ("kprobes: introduce weak arch_check_ftrace_location()
> helper function") to allow architectures to handle kprobes call site
> on their own.
>
> Recently, the only architecture (csky) to implement
> arch_check_ftrace_location() was migrated to using the common
> version.
>
> As a result, further cleanup the code to drop the weak attribute and
> rename the function to remove the architecture specific
> implementation.
>
Looks good to me.
Acked-by: Masami Hiramatsu <[email protected]>
Thanks!
> Signed-off-by: Punit Agrawal <[email protected]>
> ---
> include/linux/kprobes.h | 2 --
> kernel/kprobes.c | 4 ++--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 771013bab18a..beea9ecee187 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -369,8 +369,6 @@ static inline int arch_prepare_kprobe_ftrace(struct kprobe *p)
> }
> #endif
>
> -int arch_check_ftrace_location(struct kprobe *p);
> -
> /* Get the kprobe at this addr (if any) - called with preemption disabled */
> struct kprobe *get_kprobe(void *addr);
>
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 54d37d4ab897..b12ae6cc8dc3 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1531,7 +1531,7 @@ static inline int warn_kprobe_rereg(struct kprobe *p)
> return ret;
> }
>
> -int __weak arch_check_ftrace_location(struct kprobe *p)
> +static int check_ftrace_location(struct kprobe *p)
> {
> unsigned long ftrace_addr;
>
> @@ -1554,7 +1554,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
> {
> int ret;
>
> - ret = arch_check_ftrace_location(p);
> + ret = check_ftrace_location(p);
> if (ret)
> return ret;
> jump_label_lock();
> --
> 2.30.2
>
--
Masami Hiramatsu <[email protected]>