Make __get_wchan() use arch_stack_walk() directly to
avoid open-coding of unwind logic.
Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
---
arch/x86/kernel/process.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 3ab62ac98c2c..a6ff18fa6d5d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -1000,6 +1000,17 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
return randomize_page(mm->brk, 0x02000000);
}
+static bool get_wchan_cb(void *arg, unsigned long pc)
+{
+ unsigned long *addr = arg;
+
+ if (in_sched_functions(pc))
+ return true;
+
+ *addr = pc;
+ return false;
+}
+
/*
* Called from fs/proc with a reference on @p to find the function
* which called into schedule(). This needs to be done carefully
@@ -1008,21 +1019,12 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
*/
unsigned long __get_wchan(struct task_struct *p)
{
- struct unwind_state state;
unsigned long addr = 0;
if (!try_get_task_stack(p))
return 0;
- for (unwind_start(&state, p, NULL, NULL); !unwind_done(&state);
- unwind_next_frame(&state)) {
- addr = unwind_get_return_address(&state);
- if (!addr)
- break;
- if (in_sched_functions(addr))
- continue;
- break;
- }
+ arch_stack_walk(get_wchan_cb, &addr, p, NULL);
put_task_stack(p);
--
2.20.1On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: > Make __get_wchan() use arch_stack_walk() directly to > avoid open-coding of unwind logic. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Can we just have a shared version of __get_wchan() for all CONFIG_ARCH_STACKWALK arches? -- Josh
On Fri, Apr 07, 2023 at 10:08:22PM -0700, Josh Poimboeuf wrote: > On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: > > Make __get_wchan() use arch_stack_walk() directly to > > avoid open-coding of unwind logic. > > > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > Can we just have a shared version of __get_wchan() for all > CONFIG_ARCH_STACKWALK arches? Didn't I do that a while back ? I can't seem to actually find the patch-set though :/
On Wed, Apr 12, 2023 at 03:15:33PM +0200, Peter Zijlstra wrote: > On Fri, Apr 07, 2023 at 10:08:22PM -0700, Josh Poimboeuf wrote: > > On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: > > > Make __get_wchan() use arch_stack_walk() directly to > > > avoid open-coding of unwind logic. > > > > > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > > > Can we just have a shared version of __get_wchan() for all > > CONFIG_ARCH_STACKWALK arches? > > Didn't I do that a while back ? I can't seem to actually find the > patch-set though :/ Could be this series: https://lkml.kernel.org/r/20211022150933.883959987@infradead.org And this here: https://lore.kernel.org/lkml/CAHk-=wjHbKfck1Ws4Y0pUZ7bxdjU9eh2WK0EFsv65utfeVkT9Q@mail.gmail.com/ might be why I dropped it.. I can't remember.
On 2023/4/12 21:23, Peter Zijlstra wrote: > On Wed, Apr 12, 2023 at 03:15:33PM +0200, Peter Zijlstra wrote: >> On Fri, Apr 07, 2023 at 10:08:22PM -0700, Josh Poimboeuf wrote: >>> On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: >>>> Make __get_wchan() use arch_stack_walk() directly to >>>> avoid open-coding of unwind logic. >>>> >>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >>> >>> Can we just have a shared version of __get_wchan() for all >>> CONFIG_ARCH_STACKWALK arches? >> >> Didn't I do that a while back ? I can't seem to actually find the >> patch-set though :/ > > Could be this series: > > https://lkml.kernel.org/r/20211022150933.883959987@infradead.org Oh, I vaguely remember the beginning because I was trying to fix get_wchan() not supporting ORC unwinder on x86 [1], and then you sent a patch set, and the patch [2] in this patch set tried to implement the shared version of __get_wchan(). [1]. https://lore.kernel.org/all/20211008111626.271115116@infradead.org/ [2]. https://lore.kernel.org/all/20211008111626.392918519@infradead.org/ > > And this here: > > https://lore.kernel.org/lkml/CAHk-=wjHbKfck1Ws4Y0pUZ7bxdjU9eh2WK0EFsv65utfeVkT9Q@mail.gmail.com/ > > might be why I dropped it.. I can't remember. Didn't realize I had replied to this email before. But I also don't see why you dropped it. Looks like you have fixed the UAF problem. So do we still need to implement a shared version of __get_wchan()? If we still need it, do I need to send it again? Or just pick your previous patch directly? Both are fine to me. :) Thanks, Qi
On 2023/4/8 13:08, Josh Poimboeuf wrote: > On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: >> Make __get_wchan() use arch_stack_walk() directly to >> avoid open-coding of unwind logic. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > Can we just have a shared version of __get_wchan() for all > CONFIG_ARCH_STACKWALK arches? From a quick glance, I think it's ok, but we still need to define the arch's own get_wchan_cb(). I will try to do it. > -- Thanks, Qi
On Sat, Apr 08, 2023 at 01:36:06PM +0800, Qi Zheng wrote: > > > On 2023/4/8 13:08, Josh Poimboeuf wrote: > > On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: > > > Make __get_wchan() use arch_stack_walk() directly to > > > avoid open-coding of unwind logic. > > > > > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > > > Can we just have a shared version of __get_wchan() for all > > CONFIG_ARCH_STACKWALK arches? > > From a quick glance, I think it's ok, but we still need to define > the arch's own get_wchan_cb(). I will try to do it. Hm, why would we need to do that? -- Josh
On 2023/4/9 06:12, Josh Poimboeuf wrote: > On Sat, Apr 08, 2023 at 01:36:06PM +0800, Qi Zheng wrote: >> >> >> On 2023/4/8 13:08, Josh Poimboeuf wrote: >>> On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: >>>> Make __get_wchan() use arch_stack_walk() directly to >>>> avoid open-coding of unwind logic. >>>> >>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >>> >>> Can we just have a shared version of __get_wchan() for all >>> CONFIG_ARCH_STACKWALK arches? >> >> From a quick glance, I think it's ok, but we still need to define >> the arch's own get_wchan_cb(). I will try to do it. > > Hm, why would we need to do that? Because I see checks for count++ < 16 exist in __get_wchan() for some arches and some don't. So I'm not sure if this check can be discarded after using arch_stack_walk(). And I see that this check is retained in arm64, see [1] for details. [1]. https://github.com/torvalds/linux/commit/4f62bb7cb165f3e7b0a91279fe9dd5c56daf3457 > -- Thanks, Qi
On Sun, Apr 09, 2023 at 02:30:23PM +0800, Qi Zheng wrote: > > > On 2023/4/9 06:12, Josh Poimboeuf wrote: > > On Sat, Apr 08, 2023 at 01:36:06PM +0800, Qi Zheng wrote: > > > > > > > > > On 2023/4/8 13:08, Josh Poimboeuf wrote: > > > > On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: > > > > > Make __get_wchan() use arch_stack_walk() directly to > > > > > avoid open-coding of unwind logic. > > > > > > > > > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > > > > > > > Can we just have a shared version of __get_wchan() for all > > > > CONFIG_ARCH_STACKWALK arches? > > > > > > From a quick glance, I think it's ok, but we still need to define > > > the arch's own get_wchan_cb(). I will try to do it. > > > > Hm, why would we need to do that? > > Because I see checks for count++ < 16 exist in __get_wchan() for some > arches and some don't. So I'm not sure if this check can be discarded > after using arch_stack_walk(). And I see that this check is retained in > arm64, see [1] for details. > > [1]. https://github.com/torvalds/linux/commit/4f62bb7cb165f3e7b0a91279fe9dd5c56daf3457 That difference seems to have nothing to do with individual arch differences. The 16-check limit looks like some ancient cargo cult ritual which was copy-pasted decades ago, presumably to avoid some kind of infinite stack recursion loop in scheduler code, which should never happen. That should definitely be removed. Another good reason to unify them, to get rid of cruft like that. -- Josh
On 2023/4/12 13:20, Josh Poimboeuf wrote: > On Sun, Apr 09, 2023 at 02:30:23PM +0800, Qi Zheng wrote: >> >> >> On 2023/4/9 06:12, Josh Poimboeuf wrote: >>> On Sat, Apr 08, 2023 at 01:36:06PM +0800, Qi Zheng wrote: >>>> >>>> >>>> On 2023/4/8 13:08, Josh Poimboeuf wrote: >>>>> On Thu, Mar 30, 2023 at 04:15:52PM +0800, Qi Zheng wrote: >>>>>> Make __get_wchan() use arch_stack_walk() directly to >>>>>> avoid open-coding of unwind logic. >>>>>> >>>>>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >>>>> >>>>> Can we just have a shared version of __get_wchan() for all >>>>> CONFIG_ARCH_STACKWALK arches? >>>> >>>> From a quick glance, I think it's ok, but we still need to define >>>> the arch's own get_wchan_cb(). I will try to do it. >>> >>> Hm, why would we need to do that? >> >> Because I see checks for count++ < 16 exist in __get_wchan() for some >> arches and some don't. So I'm not sure if this check can be discarded >> after using arch_stack_walk(). And I see that this check is retained in >> arm64, see [1] for details. >> >> [1]. https://github.com/torvalds/linux/commit/4f62bb7cb165f3e7b0a91279fe9dd5c56daf3457 > > That difference seems to have nothing to do with individual arch > differences. > > The 16-check limit looks like some ancient cargo cult ritual which was > copy-pasted decades ago, presumably to avoid some kind of infinite stack > recursion loop in scheduler code, which should never happen. That > should definitely be removed. Got it. > > Another good reason to unify them, to get rid of cruft like that. OK, will try to make a shared version of __get_wchan() for all CONFIG_ARCH_STACKWALK arches. Thanks, Qi >
© 2016 - 2026 Red Hat, Inc.