[PATCH v2 0/5] kprobes: Bugfix and improvements

[PATCH v2 0/5] kprobes: Bugfix and improvements

[Punit Agrawal]

Hi,

This is the second posting of patches previously posted at
[0]. Although the patches were reviewed / acked in the previous cycle
but for some didn't end up getting picked up for this cycle.

This posting rebases the patches to 5.14-rc3 and makes some minor
improvements to the commit log in Patch 1. I've also added the tags as
appropriate from the previous posting.

It would be great if the patches can be picked up this time around.

Thanks,
Punit


[0] https://lore.kernel.org/linux-csky/20210609105019.3626677-1-punitagrawal@gmail.com/

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

[PATCH v2 1/5] kprobes: Do not use local variable when creating debugfs file

[Punit Agrawal]

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.

Since the enabled state is managed by the kprobes_all_disabled global
variable, the local variable is not needed. Fix the incorrect (and
unnecessary) usage of local variable during debugfs_file_create() by
passing NULL instead.

Fixes: bf8f6e5b3e51 ("Kprobes: The ON/OFF knob thru debugfs")
Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 790a573bbe00..1cf8bca1ea86 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2809,13 +2809,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

[PATCH v2 2/5] kprobes: Use helper to parse boolean input from userspace

[Punit Agrawal]

The "enabled" file provides a debugfs interface to arm / disarm
kprobes in the kernel. In order to parse the buffer containing the
values written from userspace, the callback manually parses the user
input to convert it to a boolean value.

As taking a string value from userspace and converting it to boolean
is a common operation, a helper kstrtobool_from_user() is already
available in the kernel. Update the callback to use the common helper
to parse the write buffer from userspace.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 1cf8bca1ea86..26fc9904c3b1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2770,30 +2770,14 @@ static ssize_t read_enabled_file_bool(struct file *file,
 static ssize_t write_enabled_file_bool(struct file *file,
 	       const char __user *user_buf, size_t count, loff_t *ppos)
 {
-	char buf[32];
-	size_t buf_size;
-	int ret = 0;
-
-	buf_size = min(count, (sizeof(buf)-1));
-	if (copy_from_user(buf, user_buf, buf_size))
-		return -EFAULT;
+	bool enable;
+	int ret;
 
-	buf[buf_size] = '\0';
-	switch (buf[0]) {
-	case 'y':
-	case 'Y':
-	case '1':
-		ret = arm_all_kprobes();
-		break;
-	case 'n':
-	case 'N':
-	case '0':
-		ret = disarm_all_kprobes();
-		break;
-	default:
-		return -EINVAL;
-	}
+	ret = kstrtobool_from_user(user_buf, count, &enable);
+	if (ret)
+		return ret;
 
+	ret = enable ? arm_all_kprobes() : disarm_all_kprobes();
 	if (ret)
 		return ret;
 
-- 
2.30.2

[PATCH v2 3/5] kprobe: Simplify prepare_kprobe() by dropping redundant version

[Punit Agrawal]

The function prepare_kprobe() is called during kprobe registration and
is responsible for ensuring any architecture related preparation for
the kprobe is done before returning.

One of two versions of prepare_kprobe() is chosen depending on the
availability of KPROBE_ON_FTRACE in the kernel configuration.

Simplify the code by dropping the version when KPROBE_ON_FTRACE is not
selected - instead relying on kprobe_ftrace() to return false when
KPROBE_ON_FTRACE is not set.

No functional change.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/kprobes.h |  5 +++++
 kernel/kprobes.c        | 23 +++++++++--------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index e4f3bfe08757..0b75549b2815 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -354,6 +354,11 @@ static inline void wait_for_kprobe_optimizer(void) { }
 extern void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 				  struct ftrace_ops *ops, struct ftrace_regs *fregs);
 extern int arch_prepare_kprobe_ftrace(struct kprobe *p);
+#else
+static inline int arch_prepare_kprobe_ftrace(struct kprobe *p)
+{
+	return -EINVAL;
+}
 #endif
 
 int arch_check_ftrace_location(struct kprobe *p);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 26fc9904c3b1..cfa9d3c263eb 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1033,15 +1033,6 @@ static struct ftrace_ops kprobe_ipmodify_ops __read_mostly = {
 static int kprobe_ipmodify_enabled;
 static int kprobe_ftrace_enabled;
 
-/* Must ensure p->addr is really on ftrace */
-static int prepare_kprobe(struct kprobe *p)
-{
-	if (!kprobe_ftrace(p))
-		return arch_prepare_kprobe(p);
-
-	return arch_prepare_kprobe_ftrace(p);
-}
-
 /* Caller must lock kprobe_mutex */
 static int __arm_kprobe_ftrace(struct kprobe *p, struct ftrace_ops *ops,
 			       int *cnt)
@@ -1113,11 +1104,6 @@ static int disarm_kprobe_ftrace(struct kprobe *p)
 		ipmodify ? &kprobe_ipmodify_enabled : &kprobe_ftrace_enabled);
 }
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
-static inline int prepare_kprobe(struct kprobe *p)
-{
-	return arch_prepare_kprobe(p);
-}
-
 static inline int arm_kprobe_ftrace(struct kprobe *p)
 {
 	return -ENODEV;
@@ -1129,6 +1115,15 @@ static inline int disarm_kprobe_ftrace(struct kprobe *p)
 }
 #endif
 
+static int prepare_kprobe(struct kprobe *p)
+{
+	/* Must ensure p->addr is really on ftrace */
+	if (kprobe_ftrace(p))
+		return arch_prepare_kprobe_ftrace(p);
+
+	return arch_prepare_kprobe(p);
+}
+
 /* Arm a kprobe with text_mutex */
 static int arm_kprobe(struct kprobe *kp)
 {
-- 
2.30.2

[PATCH v2 4/5] csky: ftrace: Drop duplicate implementation of arch_check_ftrace_location()

[Punit Agrawal]

The csky specific arch_check_ftrace_location() shadows a weak
implementation of the function in core code that offers the same
functionality but with additional error checking.

Drop the architecture specific function as a step towards further
cleanup in core code.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Acked-by: Guo Ren <guoren@kernel.org>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/csky/kernel/probes/ftrace.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
index ef2bb9bd9605..b388228abbf2 100644
--- a/arch/csky/kernel/probes/ftrace.c
+++ b/arch/csky/kernel/probes/ftrace.c
@@ -2,13 +2,6 @@
 
 #include <linux/kprobes.h>
 
-int arch_check_ftrace_location(struct kprobe *p)
-{
-	if (ftrace_location((unsigned long)p->addr))
-		p->flags |= KPROBE_FLAG_FTRACE;
-	return 0;
-}
-
 /* Ftrace callback handler for kprobes -- called under preepmt disabled */
 void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
 			   struct ftrace_ops *ops, struct ftrace_regs *fregs)
-- 
2.30.2

[PATCH v2 5/5] kprobes: Make arch_check_ftrace_location static

[Punit Agrawal]

arch_check_ftrace_location() was introduced as a weak function in
commit f7f242ff004499 ("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 <punitagrawal@gmail.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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 0b75549b2815..8a9412bb0d5e 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -361,8 +361,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 cfa9d3c263eb..30199bfcc74a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1524,7 +1524,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;
 
@@ -1547,7 +1547,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

Re: [PATCH v2 0/5] kprobes: Bugfix and improvements

[Masami Hiramatsu]

Hi Punit,

Thanks for resending this series.

Ingo, could you pick this patch too?
I'll resend my series on this series. 

Thank you,

On Tue, 27 Jul 2021 22:34:21 +0900
Punit Agrawal <punitagrawal@gmail.com> wrote:

> Hi,
> 
> This is the second posting of patches previously posted at
> [0]. Although the patches were reviewed / acked in the previous cycle
> but for some didn't end up getting picked up for this cycle.
> 
> This posting rebases the patches to 5.14-rc3 and makes some minor
> improvements to the commit log in Patch 1. I've also added the tags as
> appropriate from the previous posting.
> 
> It would be great if the patches can be picked up this time around.
> 
> Thanks,
> Punit
> 
> 
> [0] https://lore.kernel.org/linux-csky/20210609105019.3626677-1-punitagrawal@gmail.com/
> 
> 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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 4/5] csky: ftrace: Drop duplicate implementation of arch_check_ftrace_location()

[Guo Ren]

Acked, thx.

On Tue, Jul 27, 2021 at 9:35 PM Punit Agrawal <punitagrawal@gmail.com> wrote:
>
> The csky specific arch_check_ftrace_location() shadows a weak
> implementation of the function in core code that offers the same
> functionality but with additional error checking.
>
> Drop the architecture specific function as a step towards further
> cleanup in core code.
>
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Acked-by: Guo Ren <guoren@kernel.org>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/csky/kernel/probes/ftrace.c | 7 -------
>  1 file changed, 7 deletions(-)
>
> diff --git a/arch/csky/kernel/probes/ftrace.c b/arch/csky/kernel/probes/ftrace.c
> index ef2bb9bd9605..b388228abbf2 100644
> --- a/arch/csky/kernel/probes/ftrace.c
> +++ b/arch/csky/kernel/probes/ftrace.c
> @@ -2,13 +2,6 @@
>
>  #include <linux/kprobes.h>
>
> -int arch_check_ftrace_location(struct kprobe *p)
> -{
> -       if (ftrace_location((unsigned long)p->addr))
> -               p->flags |= KPROBE_FLAG_FTRACE;
> -       return 0;
> -}
> -
>  /* Ftrace callback handler for kprobes -- called under preepmt disabled */
>  void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
>                            struct ftrace_ops *ops, struct ftrace_regs *fregs)
> --
> 2.30.2
>


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Re: [PATCH v2 0/5] kprobes: Bugfix and improvements

[Punit Agrawal]

Masami Hiramatsu <mhiramat@kernel.org> writes:

> Hi Punit,
>
> Thanks for resending this series.
>
> Ingo, could you pick this patch too?
> I'll resend my series on this series.

I couldn't find the patches on any of the branches in tip - have they
been queued?

Please let me know if there anything else I need to do in order for them
to be picked up.

Thanks,
Punit

>
> Thank you,
>
> On Tue, 27 Jul 2021 22:34:21 +0900
> Punit Agrawal <punitagrawal@gmail.com> wrote:
>
>> Hi,
>> 
>> This is the second posting of patches previously posted at
>> [0]. Although the patches were reviewed / acked in the previous cycle
>> but for some didn't end up getting picked up for this cycle.
>> 
>> This posting rebases the patches to 5.14-rc3 and makes some minor
>> improvements to the commit log in Patch 1. I've also added the tags as
>> appropriate from the previous posting.
>> 
>> It would be great if the patches can be picked up this time around.
>> 
>> Thanks,
>> Punit
>> 
>> 
>> [0] https://lore.kernel.org/linux-csky/20210609105019.3626677-1-punitagrawal@gmail.com/
>> 
>> 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
>>