[PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"

Linux kernel -stable discussions
 help / color / mirror / Atom feed

* [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"
       [not found] <YeEhuGXr2B9r7mer@kroah.com>
@ 2022-01-14 10:19 ` Masami Hiramatsu
  2022-01-14 11:00   ` Greg Kroah-Hartman
  2022-01-15  4:58   ` kernel test robot
  2022-01-15 12:59 ` [PATCH v2 " Masami Hiramatsu
  1 sibling, 2 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2022-01-14 10:19 UTC (permalink / raw)
  To: stable, Greg Kroah-Hartman; +Cc: linux-kernel, Ingo Molnar, Sasha Levin

This reverts commit 77fa5e15c933a1ec812de61ad709c00aa51e96ae.

Since the upstream commit e792ff804f49720ce003b3e4c618b5d996256a18
depends on the generic kretprobe trampoline handler, which was
introduced by commit 66ada2ccae4e ("kprobes: Add generic kretprobe
trampoline handler") but that is not ported to the stable kernel
because it is not a bugfix series.
So revert this commit to fix a build error.

NOTE: I keep commit a7fe2378454c ("ia64: kprobes: Fix to pass
correct trampoline address to the handler") on the tree, that seems
just a cleanup without the original reverted commit, but it would
be better to use dereference_function_descriptor() macro instead
of accessing descriptor's field directly.


Fixes: 77fa5e15c933 ("ia64: kprobes: Use generic kretprobe trampoline handler")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/ia64/kernel/kprobes.c |   78 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 8a223d0e4918..5d2d58644378 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -396,10 +396,83 @@ static void kretprobe_trampoline(void)
 {
 }
 
+/*
+ * At this point the target function has been tricked into
+ * returning into our trampoline.  Lookup the associated instance
+ * and then:
+ *    - call the handler function
+ *    - cleanup by marking the instance as unused
+ *    - long jump back to the original return address
+ */
 int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
 {
-	regs->cr_iip = __kretprobe_trampoline_handler(regs,
-		dereference_function_descriptor(kretprobe_trampoline), NULL);
+	struct kretprobe_instance *ri = NULL;
+	struct hlist_head *head, empty_rp;
+	struct hlist_node *tmp;
+	unsigned long flags, orig_ret_address = 0;
+	unsigned long trampoline_address =
+		dereference_function_descriptor(kretprobe_trampoline);
+
+	INIT_HLIST_HEAD(&empty_rp);
+	kretprobe_hash_lock(current, &head, &flags);
+
+	/*
+	 * It is possible to have multiple instances associated with a given
+	 * task either because an multiple functions in the call path
+	 * have a return probe installed on them, and/or more than one return
+	 * return probe was registered for a target function.
+	 *
+	 * We can handle this because:
+	 *     - instances are always inserted at the head of the list
+	 *     - when multiple return probes are registered for the same
+	 *       function, the first instance's ret_addr will point to the
+	 *       real return address, and all the rest will point to
+	 *       kretprobe_trampoline
+	 */
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task != current)
+			/* another task is sharing our hash bucket */
+			continue;
+
+		orig_ret_address = (unsigned long)ri->ret_addr;
+		if (orig_ret_address != trampoline_address)
+			/*
+			 * This is the real return address. Any other
+			 * instances associated with this task are for
+			 * other calls deeper on the call stack
+			 */
+			break;
+	}
+
+	regs->cr_iip = orig_ret_address;
+
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task != current)
+			/* another task is sharing our hash bucket */
+			continue;
+
+		if (ri->rp && ri->rp->handler)
+			ri->rp->handler(ri, regs);
+
+		orig_ret_address = (unsigned long)ri->ret_addr;
+		recycle_rp_inst(ri, &empty_rp);
+
+		if (orig_ret_address != trampoline_address)
+			/*
+			 * This is the real return address. Any other
+			 * instances associated with this task are for
+			 * other calls deeper on the call stack
+			 */
+			break;
+	}
+	kretprobe_assert(ri, orig_ret_address, trampoline_address);
+
+	kretprobe_hash_unlock(current, &flags);
+
+	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
 	/*
 	 * By returning a non-zero value, we are telling
 	 * kprobe_handler() that we don't want the post_handler
@@ -412,7 +485,6 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
 	ri->ret_addr = (kprobe_opcode_t *)regs->b0;
-	ri->fp = NULL;
 
 	/* Replace the return addr with trampoline addr */
 	regs->b0 = (unsigned long)dereference_function_descriptor(kretprobe_trampoline);


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"
  2022-01-14 10:19 ` [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler" Masami Hiramatsu
@ 2022-01-14 11:00   ` Greg Kroah-Hartman
  2022-01-15  4:58   ` kernel test robot
  1 sibling, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-14 11:00 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: stable, linux-kernel, Ingo Molnar, Sasha Levin

On Fri, Jan 14, 2022 at 07:19:59PM +0900, Masami Hiramatsu wrote:
> This reverts commit 77fa5e15c933a1ec812de61ad709c00aa51e96ae.
> 
> Since the upstream commit e792ff804f49720ce003b3e4c618b5d996256a18
> depends on the generic kretprobe trampoline handler, which was
> introduced by commit 66ada2ccae4e ("kprobes: Add generic kretprobe
> trampoline handler") but that is not ported to the stable kernel
> because it is not a bugfix series.
> So revert this commit to fix a build error.
> 
> NOTE: I keep commit a7fe2378454c ("ia64: kprobes: Fix to pass
> correct trampoline address to the handler") on the tree, that seems
> just a cleanup without the original reverted commit, but it would
> be better to use dereference_function_descriptor() macro instead
> of accessing descriptor's field directly.
> 
> 
> Fixes: 77fa5e15c933 ("ia64: kprobes: Use generic kretprobe trampoline handler")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/ia64/kernel/kprobes.c |   78 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 75 insertions(+), 3 deletions(-)

Thanks for this, I'll queue it up after this round of stable releases
are out.

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"
  2022-01-14 10:19 ` [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler" Masami Hiramatsu
  2022-01-14 11:00   ` Greg Kroah-Hartman
@ 2022-01-15  4:58   ` kernel test robot
  2022-01-15 10:13     ` Masami Hiramatsu
  1 sibling, 1 reply; 6+ messages in thread
From: kernel test robot @ 2022-01-15  4:58 UTC (permalink / raw)
  To: Masami Hiramatsu, stable, Greg Kroah-Hartman
  Cc: kbuild-all, linux-kernel, Ingo Molnar, Sasha Levin

Hi Masami,

I love your patch! Perhaps something to improve:

[auto build test WARNING on stable/linux-5.4.y]

url:    https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/Revert-ia64-kprobes-Use-generic-kretprobe-trampoline-handler/20220114-182111
base:   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git linux-5.4.y
config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220115/202201151231.g2sW8oWt-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/514059de80b018e0edcf434519ff6bf41b4a519b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Masami-Hiramatsu/Revert-ia64-kprobes-Use-generic-kretprobe-trampoline-handler/20220114-182111
        git checkout 514059de80b018e0edcf434519ff6bf41b4a519b
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/ia64/kernel/kprobes.c: In function 'get_kprobe_inst':
   arch/ia64/kernel/kprobes.c:325:22: warning: variable 'template' set but not used [-Wunused-but-set-variable]
     325 |         unsigned int template;
         |                      ^~~~~~~~
   arch/ia64/kernel/kprobes.c: At top level:
   arch/ia64/kernel/kprobes.c:407:15: warning: no previous prototype for 'trampoline_probe_handler' [-Wmissing-prototypes]
     407 | int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
   arch/ia64/kernel/kprobes.c: In function 'trampoline_probe_handler':
>> arch/ia64/kernel/kprobes.c:414:17: warning: initialization of 'long unsigned int' from 'void *' makes integer from pointer without a cast [-Wint-conversion]
     414 |                 dereference_function_descriptor(kretprobe_trampoline);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for FRAME_POINTER
   Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS
   Selected by
   - FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86


vim +414 arch/ia64/kernel/kprobes.c

   320	
   321	static void __kprobes get_kprobe_inst(bundle_t *bundle, uint slot,
   322		       	unsigned long *kprobe_inst, uint *major_opcode)
   323	{
   324		unsigned long kprobe_inst_p0, kprobe_inst_p1;
 > 325		unsigned int template;
   326	
   327		template = bundle->quad0.template;
   328	
   329		switch (slot) {
   330		  case 0:
   331			*major_opcode = (bundle->quad0.slot0 >> SLOT0_OPCODE_SHIFT);
   332			*kprobe_inst = bundle->quad0.slot0;
   333			  break;
   334		  case 1:
   335			*major_opcode = (bundle->quad1.slot1_p1 >> SLOT1_p1_OPCODE_SHIFT);
   336			kprobe_inst_p0 = bundle->quad0.slot1_p0;
   337			kprobe_inst_p1 = bundle->quad1.slot1_p1;
   338			*kprobe_inst = kprobe_inst_p0 | (kprobe_inst_p1 << (64-46));
   339			break;
   340		  case 2:
   341			*major_opcode = (bundle->quad1.slot2 >> SLOT2_OPCODE_SHIFT);
   342			*kprobe_inst = bundle->quad1.slot2;
   343			break;
   344		}
   345	}
   346	
   347	/* Returns non-zero if the addr is in the Interrupt Vector Table */
   348	static int __kprobes in_ivt_functions(unsigned long addr)
   349	{
   350		return (addr >= (unsigned long)__start_ivt_text
   351			&& addr < (unsigned long)__end_ivt_text);
   352	}
   353	
   354	static int __kprobes valid_kprobe_addr(int template, int slot,
   355					       unsigned long addr)
   356	{
   357		if ((slot > 2) || ((bundle_encoding[template][1] == L) && slot > 1)) {
   358			printk(KERN_WARNING "Attempting to insert unaligned kprobe "
   359					"at 0x%lx\n", addr);
   360			return -EINVAL;
   361		}
   362	
   363		if (in_ivt_functions(addr)) {
   364			printk(KERN_WARNING "Kprobes can't be inserted inside "
   365					"IVT functions at 0x%lx\n", addr);
   366			return -EINVAL;
   367		}
   368	
   369		return 0;
   370	}
   371	
   372	static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
   373	{
   374		unsigned int i;
   375		i = atomic_add_return(1, &kcb->prev_kprobe_index);
   376		kcb->prev_kprobe[i-1].kp = kprobe_running();
   377		kcb->prev_kprobe[i-1].status = kcb->kprobe_status;
   378	}
   379	
   380	static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
   381	{
   382		unsigned int i;
   383		i = atomic_read(&kcb->prev_kprobe_index);
   384		__this_cpu_write(current_kprobe, kcb->prev_kprobe[i-1].kp);
   385		kcb->kprobe_status = kcb->prev_kprobe[i-1].status;
   386		atomic_sub(1, &kcb->prev_kprobe_index);
   387	}
   388	
   389	static void __kprobes set_current_kprobe(struct kprobe *p,
   390				struct kprobe_ctlblk *kcb)
   391	{
   392		__this_cpu_write(current_kprobe, p);
   393	}
   394	
   395	static void kretprobe_trampoline(void)
   396	{
   397	}
   398	
   399	/*
   400	 * At this point the target function has been tricked into
   401	 * returning into our trampoline.  Lookup the associated instance
   402	 * and then:
   403	 *    - call the handler function
   404	 *    - cleanup by marking the instance as unused
   405	 *    - long jump back to the original return address
   406	 */
   407	int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
   408	{
   409		struct kretprobe_instance *ri = NULL;
   410		struct hlist_head *head, empty_rp;
   411		struct hlist_node *tmp;
   412		unsigned long flags, orig_ret_address = 0;
   413		unsigned long trampoline_address =
 > 414			dereference_function_descriptor(kretprobe_trampoline);
   415	
   416		INIT_HLIST_HEAD(&empty_rp);
   417		kretprobe_hash_lock(current, &head, &flags);
   418	
   419		/*
   420		 * It is possible to have multiple instances associated with a given
   421		 * task either because an multiple functions in the call path
   422		 * have a return probe installed on them, and/or more than one return
   423		 * return probe was registered for a target function.
   424		 *
   425		 * We can handle this because:
   426		 *     - instances are always inserted at the head of the list
   427		 *     - when multiple return probes are registered for the same
   428		 *       function, the first instance's ret_addr will point to the
   429		 *       real return address, and all the rest will point to
   430		 *       kretprobe_trampoline
   431		 */
   432		hlist_for_each_entry_safe(ri, tmp, head, hlist) {
   433			if (ri->task != current)
   434				/* another task is sharing our hash bucket */
   435				continue;
   436	
   437			orig_ret_address = (unsigned long)ri->ret_addr;
   438			if (orig_ret_address != trampoline_address)
   439				/*
   440				 * This is the real return address. Any other
   441				 * instances associated with this task are for
   442				 * other calls deeper on the call stack
   443				 */
   444				break;
   445		}
   446	
   447		regs->cr_iip = orig_ret_address;
   448	
   449		hlist_for_each_entry_safe(ri, tmp, head, hlist) {
   450			if (ri->task != current)
   451				/* another task is sharing our hash bucket */
   452				continue;
   453	
   454			if (ri->rp && ri->rp->handler)
   455				ri->rp->handler(ri, regs);
   456	
   457			orig_ret_address = (unsigned long)ri->ret_addr;
   458			recycle_rp_inst(ri, &empty_rp);
   459	
   460			if (orig_ret_address != trampoline_address)
   461				/*
   462				 * This is the real return address. Any other
   463				 * instances associated with this task are for
   464				 * other calls deeper on the call stack
   465				 */
   466				break;
   467		}
   468		kretprobe_assert(ri, orig_ret_address, trampoline_address);
   469	
   470		kretprobe_hash_unlock(current, &flags);
   471	
   472		hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
   473			hlist_del(&ri->hlist);
   474			kfree(ri);
   475		}
   476		/*
   477		 * By returning a non-zero value, we are telling
   478		 * kprobe_handler() that we don't want the post_handler
   479		 * to run (and have re-enabled preemption)
   480		 */
   481		return 1;
   482	}
   483	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"
  2022-01-15  4:58   ` kernel test robot
@ 2022-01-15 10:13     ` Masami Hiramatsu
  0 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2022-01-15 10:13 UTC (permalink / raw)
  To: kernel test robot
  Cc: stable, Greg Kroah-Hartman, kbuild-all, linux-kernel, Ingo Molnar,
	Sasha Levin

On Sat, 15 Jan 2022 12:58:46 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Masami,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on stable/linux-5.4.y]
> 
> url:    https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/Revert-ia64-kprobes-Use-generic-kretprobe-trampoline-handler/20220114-182111
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git linux-5.4.y
> config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220115/202201151231.g2sW8oWt-lkp@intel.com/config)
> compiler: ia64-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/514059de80b018e0edcf434519ff6bf41b4a519b
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Masami-Hiramatsu/Revert-ia64-kprobes-Use-generic-kretprobe-trampoline-handler/20220114-182111
>         git checkout 514059de80b018e0edcf434519ff6bf41b4a519b
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>    arch/ia64/kernel/kprobes.c: In function 'get_kprobe_inst':
>    arch/ia64/kernel/kprobes.c:325:22: warning: variable 'template' set but not used [-Wunused-but-set-variable]
>      325 |         unsigned int template;
>          |                      ^~~~~~~~

This one is fixed by recent patch.

https://lore.kernel.org/all/164208575387.1590449.8278421820882450166.stgit@devnote2/T/#u

>    arch/ia64/kernel/kprobes.c: At top level:
>    arch/ia64/kernel/kprobes.c:407:15: warning: no previous prototype for 'trampoline_probe_handler' [-Wmissing-prototypes]
>      407 | int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
>          |               ^~~~~~~~~~~~~~~~~~~~~~~~
>    arch/ia64/kernel/kprobes.c: In function 'trampoline_probe_handler':
> >> arch/ia64/kernel/kprobes.c:414:17: warning: initialization of 'long unsigned int' from 'void *' makes integer from pointer without a cast [-Wint-conversion]
>      414 |                 dereference_function_descriptor(kretprobe_trampoline);
>          |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Oops. I forgot to cast it. I'll resend the patch.

Thank you,


> 
> Kconfig warnings: (for reference only)
>    WARNING: unmet direct dependencies detected for FRAME_POINTER
>    Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS
>    Selected by
>    - FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86
> 
> 
> vim +414 arch/ia64/kernel/kprobes.c
> 
>    320	
>    321	static void __kprobes get_kprobe_inst(bundle_t *bundle, uint slot,
>    322		       	unsigned long *kprobe_inst, uint *major_opcode)
>    323	{
>    324		unsigned long kprobe_inst_p0, kprobe_inst_p1;
>  > 325		unsigned int template;
>    326	
>    327		template = bundle->quad0.template;
>    328	
>    329		switch (slot) {
>    330		  case 0:
>    331			*major_opcode = (bundle->quad0.slot0 >> SLOT0_OPCODE_SHIFT);
>    332			*kprobe_inst = bundle->quad0.slot0;
>    333			  break;
>    334		  case 1:
>    335			*major_opcode = (bundle->quad1.slot1_p1 >> SLOT1_p1_OPCODE_SHIFT);
>    336			kprobe_inst_p0 = bundle->quad0.slot1_p0;
>    337			kprobe_inst_p1 = bundle->quad1.slot1_p1;
>    338			*kprobe_inst = kprobe_inst_p0 | (kprobe_inst_p1 << (64-46));
>    339			break;
>    340		  case 2:
>    341			*major_opcode = (bundle->quad1.slot2 >> SLOT2_OPCODE_SHIFT);
>    342			*kprobe_inst = bundle->quad1.slot2;
>    343			break;
>    344		}
>    345	}
>    346	
>    347	/* Returns non-zero if the addr is in the Interrupt Vector Table */
>    348	static int __kprobes in_ivt_functions(unsigned long addr)
>    349	{
>    350		return (addr >= (unsigned long)__start_ivt_text
>    351			&& addr < (unsigned long)__end_ivt_text);
>    352	}
>    353	
>    354	static int __kprobes valid_kprobe_addr(int template, int slot,
>    355					       unsigned long addr)
>    356	{
>    357		if ((slot > 2) || ((bundle_encoding[template][1] == L) && slot > 1)) {
>    358			printk(KERN_WARNING "Attempting to insert unaligned kprobe "
>    359					"at 0x%lx\n", addr);
>    360			return -EINVAL;
>    361		}
>    362	
>    363		if (in_ivt_functions(addr)) {
>    364			printk(KERN_WARNING "Kprobes can't be inserted inside "
>    365					"IVT functions at 0x%lx\n", addr);
>    366			return -EINVAL;
>    367		}
>    368	
>    369		return 0;
>    370	}
>    371	
>    372	static void __kprobes save_previous_kprobe(struct kprobe_ctlblk *kcb)
>    373	{
>    374		unsigned int i;
>    375		i = atomic_add_return(1, &kcb->prev_kprobe_index);
>    376		kcb->prev_kprobe[i-1].kp = kprobe_running();
>    377		kcb->prev_kprobe[i-1].status = kcb->kprobe_status;
>    378	}
>    379	
>    380	static void __kprobes restore_previous_kprobe(struct kprobe_ctlblk *kcb)
>    381	{
>    382		unsigned int i;
>    383		i = atomic_read(&kcb->prev_kprobe_index);
>    384		__this_cpu_write(current_kprobe, kcb->prev_kprobe[i-1].kp);
>    385		kcb->kprobe_status = kcb->prev_kprobe[i-1].status;
>    386		atomic_sub(1, &kcb->prev_kprobe_index);
>    387	}
>    388	
>    389	static void __kprobes set_current_kprobe(struct kprobe *p,
>    390				struct kprobe_ctlblk *kcb)
>    391	{
>    392		__this_cpu_write(current_kprobe, p);
>    393	}
>    394	
>    395	static void kretprobe_trampoline(void)
>    396	{
>    397	}
>    398	
>    399	/*
>    400	 * At this point the target function has been tricked into
>    401	 * returning into our trampoline.  Lookup the associated instance
>    402	 * and then:
>    403	 *    - call the handler function
>    404	 *    - cleanup by marking the instance as unused
>    405	 *    - long jump back to the original return address
>    406	 */
>    407	int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
>    408	{
>    409		struct kretprobe_instance *ri = NULL;
>    410		struct hlist_head *head, empty_rp;
>    411		struct hlist_node *tmp;
>    412		unsigned long flags, orig_ret_address = 0;
>    413		unsigned long trampoline_address =
>  > 414			dereference_function_descriptor(kretprobe_trampoline);
>    415	
>    416		INIT_HLIST_HEAD(&empty_rp);
>    417		kretprobe_hash_lock(current, &head, &flags);
>    418	
>    419		/*
>    420		 * It is possible to have multiple instances associated with a given
>    421		 * task either because an multiple functions in the call path
>    422		 * have a return probe installed on them, and/or more than one return
>    423		 * return probe was registered for a target function.
>    424		 *
>    425		 * We can handle this because:
>    426		 *     - instances are always inserted at the head of the list
>    427		 *     - when multiple return probes are registered for the same
>    428		 *       function, the first instance's ret_addr will point to the
>    429		 *       real return address, and all the rest will point to
>    430		 *       kretprobe_trampoline
>    431		 */
>    432		hlist_for_each_entry_safe(ri, tmp, head, hlist) {
>    433			if (ri->task != current)
>    434				/* another task is sharing our hash bucket */
>    435				continue;
>    436	
>    437			orig_ret_address = (unsigned long)ri->ret_addr;
>    438			if (orig_ret_address != trampoline_address)
>    439				/*
>    440				 * This is the real return address. Any other
>    441				 * instances associated with this task are for
>    442				 * other calls deeper on the call stack
>    443				 */
>    444				break;
>    445		}
>    446	
>    447		regs->cr_iip = orig_ret_address;
>    448	
>    449		hlist_for_each_entry_safe(ri, tmp, head, hlist) {
>    450			if (ri->task != current)
>    451				/* another task is sharing our hash bucket */
>    452				continue;
>    453	
>    454			if (ri->rp && ri->rp->handler)
>    455				ri->rp->handler(ri, regs);
>    456	
>    457			orig_ret_address = (unsigned long)ri->ret_addr;
>    458			recycle_rp_inst(ri, &empty_rp);
>    459	
>    460			if (orig_ret_address != trampoline_address)
>    461				/*
>    462				 * This is the real return address. Any other
>    463				 * instances associated with this task are for
>    464				 * other calls deeper on the call stack
>    465				 */
>    466				break;
>    467		}
>    468		kretprobe_assert(ri, orig_ret_address, trampoline_address);
>    469	
>    470		kretprobe_hash_unlock(current, &flags);
>    471	
>    472		hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
>    473			hlist_del(&ri->hlist);
>    474			kfree(ri);
>    475		}
>    476		/*
>    477		 * By returning a non-zero value, we are telling
>    478		 * kprobe_handler() that we don't want the post_handler
>    479		 * to run (and have re-enabled preemption)
>    480		 */
>    481		return 1;
>    482	}
>    483	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"
       [not found] <YeEhuGXr2B9r7mer@kroah.com>
  2022-01-14 10:19 ` [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler" Masami Hiramatsu
@ 2022-01-15 12:59 ` Masami Hiramatsu
  2022-01-24 15:09   ` Greg Kroah-Hartman
  1 sibling, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2022-01-15 12:59 UTC (permalink / raw)
  To: stable, Greg Kroah-Hartman; +Cc: linux-kernel, Ingo Molnar, Sasha Levin

This reverts commit 77fa5e15c933a1ec812de61ad709c00aa51e96ae.

Since the upstream commit e792ff804f49720ce003b3e4c618b5d996256a18
depends on the generic kretprobe trampoline handler, which was
introduced by commit 66ada2ccae4e ("kprobes: Add generic kretprobe
trampoline handler") but that is not ported to the stable kernel
because it is not a bugfix series.
So revert this commit to fix a build error.

NOTE: I keep commit a7fe2378454c ("ia64: kprobes: Fix to pass
correct trampoline address to the handler") on the tree, that seems
just a cleanup without the original reverted commit, but it would
be better to use dereference_function_descriptor() macro instead
of accessing descriptor's field directly.

Fixes: 77fa5e15c933 ("ia64: kprobes: Use generic kretprobe trampoline handler")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
  Changes in v2:
   - fix the lack of type casting for dereference_function_descriptor().
---
 arch/ia64/kernel/kprobes.c |   78 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
index 8a223d0e4918..fa10d51f6217 100644
--- a/arch/ia64/kernel/kprobes.c
+++ b/arch/ia64/kernel/kprobes.c
@@ -396,10 +396,83 @@ static void kretprobe_trampoline(void)
 {
 }
 
+/*
+ * At this point the target function has been tricked into
+ * returning into our trampoline.  Lookup the associated instance
+ * and then:
+ *    - call the handler function
+ *    - cleanup by marking the instance as unused
+ *    - long jump back to the original return address
+ */
 int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
 {
-	regs->cr_iip = __kretprobe_trampoline_handler(regs,
-		dereference_function_descriptor(kretprobe_trampoline), NULL);
+	struct kretprobe_instance *ri = NULL;
+	struct hlist_head *head, empty_rp;
+	struct hlist_node *tmp;
+	unsigned long flags, orig_ret_address = 0;
+	unsigned long trampoline_address =
+		(unsigned long)dereference_function_descriptor(kretprobe_trampoline);
+
+	INIT_HLIST_HEAD(&empty_rp);
+	kretprobe_hash_lock(current, &head, &flags);
+
+	/*
+	 * It is possible to have multiple instances associated with a given
+	 * task either because an multiple functions in the call path
+	 * have a return probe installed on them, and/or more than one return
+	 * return probe was registered for a target function.
+	 *
+	 * We can handle this because:
+	 *     - instances are always inserted at the head of the list
+	 *     - when multiple return probes are registered for the same
+	 *       function, the first instance's ret_addr will point to the
+	 *       real return address, and all the rest will point to
+	 *       kretprobe_trampoline
+	 */
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task != current)
+			/* another task is sharing our hash bucket */
+			continue;
+
+		orig_ret_address = (unsigned long)ri->ret_addr;
+		if (orig_ret_address != trampoline_address)
+			/*
+			 * This is the real return address. Any other
+			 * instances associated with this task are for
+			 * other calls deeper on the call stack
+			 */
+			break;
+	}
+
+	regs->cr_iip = orig_ret_address;
+
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task != current)
+			/* another task is sharing our hash bucket */
+			continue;
+
+		if (ri->rp && ri->rp->handler)
+			ri->rp->handler(ri, regs);
+
+		orig_ret_address = (unsigned long)ri->ret_addr;
+		recycle_rp_inst(ri, &empty_rp);
+
+		if (orig_ret_address != trampoline_address)
+			/*
+			 * This is the real return address. Any other
+			 * instances associated with this task are for
+			 * other calls deeper on the call stack
+			 */
+			break;
+	}
+	kretprobe_assert(ri, orig_ret_address, trampoline_address);
+
+	kretprobe_hash_unlock(current, &flags);
+
+	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
 	/*
 	 * By returning a non-zero value, we are telling
 	 * kprobe_handler() that we don't want the post_handler
@@ -412,7 +485,6 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
 	ri->ret_addr = (kprobe_opcode_t *)regs->b0;
-	ri->fp = NULL;
 
 	/* Replace the return addr with trampoline addr */
 	regs->b0 = (unsigned long)dereference_function_descriptor(kretprobe_trampoline);


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler"
  2022-01-15 12:59 ` [PATCH v2 " Masami Hiramatsu
@ 2022-01-24 15:09   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-01-24 15:09 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: stable, linux-kernel, Ingo Molnar, Sasha Levin

On Sat, Jan 15, 2022 at 09:59:16PM +0900, Masami Hiramatsu wrote:
> This reverts commit 77fa5e15c933a1ec812de61ad709c00aa51e96ae.
> 
> Since the upstream commit e792ff804f49720ce003b3e4c618b5d996256a18
> depends on the generic kretprobe trampoline handler, which was
> introduced by commit 66ada2ccae4e ("kprobes: Add generic kretprobe
> trampoline handler") but that is not ported to the stable kernel
> because it is not a bugfix series.
> So revert this commit to fix a build error.
> 
> NOTE: I keep commit a7fe2378454c ("ia64: kprobes: Fix to pass
> correct trampoline address to the handler") on the tree, that seems
> just a cleanup without the original reverted commit, but it would
> be better to use dereference_function_descriptor() macro instead
> of accessing descriptor's field directly.
> 
> Fixes: 77fa5e15c933 ("ia64: kprobes: Use generic kretprobe trampoline handler")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>   Changes in v2:
>    - fix the lack of type casting for dereference_function_descriptor().
> ---
>  arch/ia64/kernel/kprobes.c |   78 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c
> index 8a223d0e4918..fa10d51f6217 100644
> --- a/arch/ia64/kernel/kprobes.c
> +++ b/arch/ia64/kernel/kprobes.c
> @@ -396,10 +396,83 @@ static void kretprobe_trampoline(void)
>  {
>  }
>  
> +/*
> + * At this point the target function has been tricked into
> + * returning into our trampoline.  Lookup the associated instance
> + * and then:
> + *    - call the handler function
> + *    - cleanup by marking the instance as unused
> + *    - long jump back to the original return address
> + */
>  int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
>  {
> -	regs->cr_iip = __kretprobe_trampoline_handler(regs,
> -		dereference_function_descriptor(kretprobe_trampoline), NULL);
> +	struct kretprobe_instance *ri = NULL;
> +	struct hlist_head *head, empty_rp;
> +	struct hlist_node *tmp;
> +	unsigned long flags, orig_ret_address = 0;
> +	unsigned long trampoline_address =
> +		(unsigned long)dereference_function_descriptor(kretprobe_trampoline);
> +
> +	INIT_HLIST_HEAD(&empty_rp);
> +	kretprobe_hash_lock(current, &head, &flags);
> +
> +	/*
> +	 * It is possible to have multiple instances associated with a given
> +	 * task either because an multiple functions in the call path
> +	 * have a return probe installed on them, and/or more than one return
> +	 * return probe was registered for a target function.
> +	 *
> +	 * We can handle this because:
> +	 *     - instances are always inserted at the head of the list
> +	 *     - when multiple return probes are registered for the same
> +	 *       function, the first instance's ret_addr will point to the
> +	 *       real return address, and all the rest will point to
> +	 *       kretprobe_trampoline
> +	 */
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->task != current)
> +			/* another task is sharing our hash bucket */
> +			continue;
> +
> +		orig_ret_address = (unsigned long)ri->ret_addr;
> +		if (orig_ret_address != trampoline_address)
> +			/*
> +			 * This is the real return address. Any other
> +			 * instances associated with this task are for
> +			 * other calls deeper on the call stack
> +			 */
> +			break;
> +	}
> +
> +	regs->cr_iip = orig_ret_address;
> +
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->task != current)
> +			/* another task is sharing our hash bucket */
> +			continue;
> +
> +		if (ri->rp && ri->rp->handler)
> +			ri->rp->handler(ri, regs);
> +
> +		orig_ret_address = (unsigned long)ri->ret_addr;
> +		recycle_rp_inst(ri, &empty_rp);
> +
> +		if (orig_ret_address != trampoline_address)
> +			/*
> +			 * This is the real return address. Any other
> +			 * instances associated with this task are for
> +			 * other calls deeper on the call stack
> +			 */
> +			break;
> +	}
> +	kretprobe_assert(ri, orig_ret_address, trampoline_address);
> +
> +	kretprobe_hash_unlock(current, &flags);
> +
> +	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> +		hlist_del(&ri->hlist);
> +		kfree(ri);
> +	}
>  	/*
>  	 * By returning a non-zero value, we are telling
>  	 * kprobe_handler() that we don't want the post_handler
> @@ -412,7 +485,6 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  				      struct pt_regs *regs)
>  {
>  	ri->ret_addr = (kprobe_opcode_t *)regs->b0;
> -	ri->fp = NULL;
>  
>  	/* Replace the return addr with trampoline addr */
>  	regs->b0 = (unsigned long)dereference_function_descriptor(kretprobe_trampoline);
> 

Now queued up, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-01-24 15:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <YeEhuGXr2B9r7mer@kroah.com>
2022-01-14 10:19 ` [PATCH 5.4.y] Revert "ia64: kprobes: Use generic kretprobe trampoline handler" Masami Hiramatsu
2022-01-14 11:00   ` Greg Kroah-Hartman
2022-01-15  4:58   ` kernel test robot
2022-01-15 10:13     ` Masami Hiramatsu
2022-01-15 12:59 ` [PATCH v2 " Masami Hiramatsu
2022-01-24 15:09   ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox