diff mbox series

[2/2] x86: make __get_wchan() use arch_stack_walk()

Message ID 20230330081552.54178-3-zhengqi.arch@bytedance.com
State New
Headers
Series some fixes and cleanups for x86 |

Commit Message

Qi Zheng March 30, 2023, 8:15 a.m. UTC 
  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(-)

Comments

Josh Poimboeuf April 8, 2023, 5:08 a.m. UTC | #1 
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?
Qi Zheng April 8, 2023, 5:36 a.m. UTC | #2 
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.

>
Josh Poimboeuf April 8, 2023, 10:12 p.m. UTC | #3 
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?
Qi Zheng April 9, 2023, 6:30 a.m. UTC | #4 
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

>
Josh Poimboeuf April 12, 2023, 5:20 a.m. UTC | #5 
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.
Qi Zheng April 12, 2023, 7:10 a.m. UTC | #6 
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

>
Peter Zijlstra April 12, 2023, 1:15 p.m. UTC | #7 
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 :/
Peter Zijlstra April 12, 2023, 1:23 p.m. UTC | #8 
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.
Qi Zheng April 12, 2023, 5:24 p.m. UTC | #9 
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
diff mbox series

Patch

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);