[PATCH 0/5] kprobes: Bugfix and improvements

[PATCH 0/5] kprobes: Bugfix and improvements

[Punit Agrawal]

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

[PATCH 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.

Fix the incorrect (and unnecessary) usage of local variable during
debugfs_file_create() by passing NULL instead.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
---
 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

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

[Masami Hiramatsu]

On Wed,  9 Jun 2021 19:50:15 +0900
Punit Agrawal <punitagrawal@gmail.com> wrote:

> 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.
> 

Good catch! Since the enabled state is managed by the kprobes_all_disabled
global variable, it is not needed.

Fixes: bf8f6e5b3e51 ("Kprobes: The ON/OFF knob thru debugfs")
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> ---
>  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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Punit Agrawal]

Hi Masami,

Masami Hiramatsu <mhiramat@kernel.org> writes:

> On Wed,  9 Jun 2021 19:50:15 +0900
> Punit Agrawal <punitagrawal@gmail.com> wrote:
>
>> 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.
>> 
>
> Good catch! Since the enabled state is managed by the kprobes_all_disabled
> global variable, it is not needed.
>
> Fixes: bf8f6e5b3e51 ("Kprobes: The ON/OFF knob thru debugfs")
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks a lot for reviewing the patches.

I am assuming the tags can be picked up when applying. Let me know if I
need to resend.

Thanks,
Punit

>
> Thank you!
>
>> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
>> ---
>>  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
>> 

[RFC PATCH 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>
---
 kernel/kprobes.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index fdb1ea2e963b..1a11d3c411bf 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2777,30 +2777,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

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

[Masami Hiramatsu]

On Wed,  9 Jun 2021 19:50:16 +0900
Punit Agrawal <punitagrawal@gmail.com> wrote:

> 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.

Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> 
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> ---
>  kernel/kprobes.c | 28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index fdb1ea2e963b..1a11d3c411bf 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2777,30 +2777,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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[RFC PATCH 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>
---
 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 1883a4a9f16a..771013bab18a 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -362,6 +362,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 1a11d3c411bf..54d37d4ab897 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1022,15 +1022,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)
@@ -1102,11 +1093,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;
@@ -1118,6 +1104,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

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

[Masami Hiramatsu]

On Wed,  9 Jun 2021 19:50:17 +0900
Punit Agrawal <punitagrawal@gmail.com> wrote:

> 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.
> 

OK, since kprobe_ftrace() just checks a flag is set or not,
it is always usable in kprobes.c.
Looks good to me :)

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!


> No functional change.
> 
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> ---
>  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 1883a4a9f16a..771013bab18a 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -362,6 +362,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 1a11d3c411bf..54d37d4ab897 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1022,15 +1022,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)
> @@ -1102,11 +1093,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;
> @@ -1118,6 +1104,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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[RFC PATCH 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>
Cc: Guo Ren <guoren@linux.alibaba.com>
---
 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

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

[Guo Ren]

csky using -mcount not -fpatchable-function-entry, so
                /* Given address is not on the instruction boundary */
                if ((unsigned long)p->addr != ftrace_addr)
                        return -EILSEQ;
all right?

On Wed, Jun 9, 2021 at 6:51 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>
> Cc: Guo Ren <guoren@linux.alibaba.com>
> ---
>  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: [RFC PATCH 4/5] csky: ftrace: Drop duplicate implementation of arch_check_ftrace_location()

[Masami Hiramatsu]

Hi Guo,

On Wed, 9 Jun 2021 20:33:18 +0800
Guo Ren <guoren@kernel.org> wrote:

> csky using -mcount not -fpatchable-function-entry, so
>                 /* Given address is not on the instruction boundary */
>                 if ((unsigned long)p->addr != ftrace_addr)
>                         return -EILSEQ;
> all right?

Even if -mcount is used, that check is still needed since the
ftrace hooked address will be the ftrace_addr. If user tries to
probe the second instruction in mcount code, kprobes needs to stop it.

Thank you,

> 
> On Wed, Jun 9, 2021 at 6:51 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>
> > Cc: Guo Ren <guoren@linux.alibaba.com>
> > ---
> >  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/


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Guo Ren]

On Wed, Jun 9, 2021 at 10:30 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Guo,
>
> On Wed, 9 Jun 2021 20:33:18 +0800
> Guo Ren <guoren@kernel.org> wrote:
>
> > csky using -mcount not -fpatchable-function-entry, so
> >                 /* Given address is not on the instruction boundary */
> >                 if ((unsigned long)p->addr != ftrace_addr)
> >                         return -EILSEQ;
> > all right?
>
> Even if -mcount is used, that check is still needed since the
> ftrace hooked address will be the ftrace_addr. If user tries to
> probe the second instruction in mcount code, kprobes needs to stop it.
Make sense. It guarantees p->addr is the first instruction of function
-mcount call site.

Thx

Acked-by: Guo Ren <guoren@kernel.org>

>
> Thank you,
>
> >
> > On Wed, Jun 9, 2021 at 6:51 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>
> > > Cc: Guo Ren <guoren@linux.alibaba.com>
> > > ---
> > >  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/
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>



-- 
Best Regards
 Guo Ren

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

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

[Masami Hiramatsu]

On Wed,  9 Jun 2021 19:50:18 +0900
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.
> 

Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Cc: Guo Ren <guoren@linux.alibaba.com>
> ---
>  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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Punit Agrawal]

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 <punitagrawal@gmail.com>
---
 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

Re: [RFC PATCH 5/5] kprobes: Make arch_check_ftrace_location static

[Masami Hiramatsu]

On Wed,  9 Jun 2021 19:50:19 +0900
Punit Agrawal <punitagrawal@gmail.com> 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 <mhiramat@kernel.org>

Thanks!

> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> ---
>  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 <mhiramat@kernel.org>