Message ID | 20230330081552.54178-3-zhengqi.arch@bytedance.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp962690vqo; Thu, 30 Mar 2023 01:21:31 -0700 (PDT) X-Google-Smtp-Source: AKy350am1CInJt6e6tNp5tiwma5Ws2o+UZJJ5d9Tc6vFvIYe7zCe0+xQAHyTjRodKSE8f214skA/ X-Received: by 2002:a17:906:f212:b0:8f6:dc49:337f with SMTP id gt18-20020a170906f21200b008f6dc49337fmr21641363ejb.43.1680164491256; Thu, 30 Mar 2023 01:21:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680164491; cv=none; d=google.com; s=arc-20160816; b=xS8TrpVvdp7SKgTWSxQP9kwzaZmPBYEXwBSeUSMmiUFMEYrdlRjFrVgPPNT0WzRSdA 926LfDWuUu8rpdtsHrf93ckamj//4pGvev3w3ZSLDUOgn2gwLsh3M8ZOr38qAmegzbxA TTLH5IbAyWNIipMo8SPIuHb8uXs115Gy3m3GbloBOIFP6sLzhYZ+6DI6R0as7Ic9xPIS El9ZI+F/bGTbpHSnef+FdBwolJCYQRQjKGqq+i4AM62duGyMBLJCSFkJbg2XagSOFOX1 mlxsRNTEgwKyZWjDYBMcLcxSGmqAwC2UU6whJ5RMPcA1VpE+vUxrEHI83hmLAmV2VQ8X ukhQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=BOI7aF7UFpggKDWXdppF1ANUfbw7Suxw6cG2yN/yOEM=; b=iNcN1dqpnI8D2dsVz6zN7/6rLfJX+SE8UYlGPfncSSpt0B/uX+2NtTttYa++URHaSp bqA73X+1UUxDExWr+Xn72qHBZk+o6leWUNDhGKRZO8jrHY0zojozDNgpmcCHIzixGftq QUFfE7n5wVF7sEgPvwYkXeF+ISlNXroKg7/FLVafukFzy+ft33F1hseues53Z63WGGAK vvGTWp1p/GL5/6J3Pg5/9vdYFC0ljES73lFFH/vCYJdy64+CViOQOJBxPKibzPlJuMbZ FNf7XbMR8Pgo1KSRrSa1T1n+YVE+oT8/4xDWt+bigv/EVzYiP43sG/nLWdBrId0kDI1I MU6Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bytedance.com header.s=google header.b=CieFy9DL; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=bytedance.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ti12-20020a170907c20c00b0093e6f520acesi208998ejc.832.2023.03.30.01.21.07; Thu, 30 Mar 2023 01:21:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@bytedance.com header.s=google header.b=CieFy9DL; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=bytedance.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230422AbjC3IRQ (ORCPT <rfc822;rua109.linux@gmail.com> + 99 others); Thu, 30 Mar 2023 04:17:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58392 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230348AbjC3IQr (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 30 Mar 2023 04:16:47 -0400 Received: from mail-pl1-x634.google.com (mail-pl1-x634.google.com [IPv6:2607:f8b0:4864:20::634]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E1ED372B1 for <linux-kernel@vger.kernel.org>; Thu, 30 Mar 2023 01:16:22 -0700 (PDT) Received: by mail-pl1-x634.google.com with SMTP id o2so17386738plg.4 for <linux-kernel@vger.kernel.org>; Thu, 30 Mar 2023 01:16:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1680164182; x=1682756182; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=BOI7aF7UFpggKDWXdppF1ANUfbw7Suxw6cG2yN/yOEM=; b=CieFy9DLNJ6vTy/qq9cI1yNZ5APxTLFi+sxUwDVTaTts3ukMIaD1avDKCbi++RVffn SuRcDl5yykEfAIRvIWB3QVZdFyEUsKk3/Jk/7v3N0ZJ0v+atSYfhc/Fa3OM/lIJal5ef aeOgSbclMbDKzCbGDtj4rpv2j/cjNNgmPgYZra6lgV0jrRxJUBuDCU8iY4VGogaVnOfx HGjDLXA0fR8OQ//l79Goq875dfMG/A5UTUwOcoEWRAqbrq4bjoduHwGbN1E0CAqEFO/B H3daZ4DAzSciC2z52E/7LHJnXwYzCId6+d7eH1npqd0/6xkTglDnJS5hv3Jl9+KQUXvb xbjg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680164182; x=1682756182; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BOI7aF7UFpggKDWXdppF1ANUfbw7Suxw6cG2yN/yOEM=; b=s6tsMHOdngkqm8403k+Wrfr5RoDALD7nvVtS2g69zI1lUB3wuMXMoHSmpMpCmvB47K ScBN7xBPQxKJMlkdSW7RnRT8h644Xj1QLQ5FmrTCVqiMPwIW0pI1gnOE5DLpZZNzvtHe lMfbPRV26WkqU2HCQrN+Yt0dd+4X7xReGb3DHh69Bk7rzJw4o8YV4tsS9PcOThjyfn9F Bu7U2AVmPorqrnY07O/KOCqKG5FASltEeAaRKhF5oFPCwRoCjQzxA9EcuPWl4L6+3rAL LR4fH/w6/4VDRTPRUInlqeY5RB53CxEPpJw6fiwEtU2TSwKa8T6FeejSRGzhDAf6sB55 mgBg== X-Gm-Message-State: AAQBX9d72xS8JN4btXYED+tG3fdzvdojTP3DkUT9ekdawR10WIyZx2zP yxq7vCAW9kdwWrfihn1V2HfCdw== X-Received: by 2002:a17:90a:788f:b0:23d:2b53:1ae2 with SMTP id x15-20020a17090a788f00b0023d2b531ae2mr1491639pjk.3.1680164182265; Thu, 30 Mar 2023 01:16:22 -0700 (PDT) Received: from C02DW0BEMD6R.bytedance.net ([139.177.225.245]) by smtp.gmail.com with ESMTPSA id i13-20020a17090a138d00b0023cd53e7706sm2630837pja.47.2023.03.30.01.16.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Mar 2023 01:16:21 -0700 (PDT) From: Qi Zheng <zhengqi.arch@bytedance.com> To: peterz@infradead.org, keescook@chromium.org, jpoimboe@kernel.org, dave.hansen@linux.intel.com, bp@alien8.de, mingo@redhat.com, tglx@linutronix.de, rostedt@goodmis.org Cc: x86@kernel.org, linux-kernel@vger.kernel.org, Qi Zheng <zhengqi.arch@bytedance.com> Subject: [PATCH 2/2] x86: make __get_wchan() use arch_stack_walk() Date: Thu, 30 Mar 2023 16:15:52 +0800 Message-Id: <20230330081552.54178-3-zhengqi.arch@bytedance.com> X-Mailer: git-send-email 2.24.3 (Apple Git-128) In-Reply-To: <20230330081552.54178-1-zhengqi.arch@bytedance.com> References: <20230330081552.54178-1-zhengqi.arch@bytedance.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761780161507376125?= X-GMAIL-MSGID: =?utf-8?q?1761780161507376125?= |
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
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?
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. >
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?
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 >
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.
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 >
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
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);