From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1EEDAC433EF for ; Sat, 5 Mar 2022 06:06:31 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 623098D0012; Sat, 5 Mar 2022 01:06:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5D16B8D0001; Sat, 5 Mar 2022 01:06:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4C0358D0012; Sat, 5 Mar 2022 01:06:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.26]) by kanga.kvack.org (Postfix) with ESMTP id 395B88D0001 for ; Sat, 5 Mar 2022 01:06:31 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 019A423E59 for ; Sat, 5 Mar 2022 06:06:30 +0000 (UTC) X-FDA: 79209298182.13.06D144C Received: from out30-131.freemail.mail.aliyun.com (out30-131.freemail.mail.aliyun.com [115.124.30.131]) by imf13.hostedemail.com (Postfix) with ESMTP id 68DE320005 for ; Sat, 5 Mar 2022 06:06:29 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R651e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04394;MF=dtcccc@linux.alibaba.com;NM=1;PH=DS;RN=7;SR=0;TI=SMTPD_---0V6FfDh8_1646460384; Received: from 192.168.0.205(mailfrom:dtcccc@linux.alibaba.com fp:SMTPD_---0V6FfDh8_1646460384) by smtp.aliyun-inc.com(127.0.0.1); Sat, 05 Mar 2022 14:06:25 +0800 Message-ID: Date: Sat, 5 Mar 2022 14:06:23 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [RFC PATCH 1/2] kfence: Allow re-enabling KFENCE after system startup Content-Language: en-US From: Tianchen Ding To: Marco Elver Cc: Alexander Potapenko , Dmitry Vyukov , Andrew Morton , kasan-dev@googlegroups.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20220303031505.28495-1-dtcccc@linux.alibaba.com> <20220303031505.28495-2-dtcccc@linux.alibaba.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 68DE320005 X-Stat-Signature: gx8jtu567bcmfs1a1r9mfaqnaozj9wdk Authentication-Results: imf13.hostedemail.com; dkim=none; spf=pass (imf13.hostedemail.com: domain of dtcccc@linux.alibaba.com designates 115.124.30.131 as permitted sender) smtp.mailfrom=dtcccc@linux.alibaba.com; dmarc=pass (policy=none) header.from=alibaba.com X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1646460389-389945 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 2022/3/5 13:26, Tianchen Ding wrote: > On 2022/3/5 02:13, Marco Elver wrote: >> On Thu, 3 Mar 2022 at 04:15, Tianchen Ding >> wrote: >>> >>> If once KFENCE is disabled by: >>> echo 0 > /sys/module/kfence/parameters/sample_interval >>> KFENCE could never be re-enabled until next rebooting. >>> >>> Allow re-enabling it by writing a positive num to sample_interval. >>> >>> Signed-off-by: Tianchen Ding >> >> The only problem I see with this is if KFENCE was disabled because of >> a KFENCE_WARN_ON(). See below. >> >>> --- >>>   mm/kfence/core.c | 16 ++++++++++++++-- >>>   1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/mm/kfence/core.c b/mm/kfence/core.c >>> index 13128fa13062..19eb123c0bba 100644 >>> --- a/mm/kfence/core.c >>> +++ b/mm/kfence/core.c >>> @@ -55,6 +55,7 @@ EXPORT_SYMBOL_GPL(kfence_sample_interval); /* >>> Export for test modules. */ >>>   #endif >>>   #define MODULE_PARAM_PREFIX "kfence." >>> >>> +static int kfence_enable_late(void); >>>   static int param_set_sample_interval(const char *val, const struct >>> kernel_param *kp) >>>   { >>>          unsigned long num; >>> @@ -65,10 +66,11 @@ static int param_set_sample_interval(const char >>> *val, const struct kernel_param >>> >>>          if (!num) /* Using 0 to indicate KFENCE is disabled. */ >>>                  WRITE_ONCE(kfence_enabled, false); >>> -       else if (!READ_ONCE(kfence_enabled) && system_state != >>> SYSTEM_BOOTING) >>> -               return -EINVAL; /* Cannot (re-)enable KFENCE >>> on-the-fly. */ >>> >>>          *((unsigned long *)kp->arg) = num; >>> + >>> +       if (num && !READ_ONCE(kfence_enabled) && system_state != >>> SYSTEM_BOOTING) >> >> Should probably have an 'old_sample_interval = *((unsigned long >> *)kp->arg)' somewhere before, and add a '&& !old_sample_interval', >> because if old_sample_interval!=0 then KFENCE was disabled due to a >> KFENCE_WARN_ON(). Also in this case, it should return -EINVAL. So you >> want a flow like this: >> >> old_sample_interval = ...; >> ... >> if (num && !READ_ONCE(kfence_enabled) && system_state != SYSTEM_BOOTING) >>    return old_sample_interval ? -EINVAL : kfence_enable_late(); >> ... >> > > Because sample_interval will used by delayed_work, we must put setting > sample_interval before enabling KFENCE. > So the order would be: > > old_sample_interval = sample_interval; > sample_interval = num; > if (...) kfence_enable_late(); > > This may be bypassed after KFENCE_WARN_ON() happens, if we first write > 0, and then write 100 to it. > > How about this one: > >     if (ret < 0) >         return ret; > > +    /* Cannot set sample_interval after KFENCE_WARN_ON(). */ > +    if (unlikely(*((unsigned long *)kp->arg) && > !READ_ONCE(kfence_enabled))) > +        return -EINVAL; > + >     if (!num) /* Using 0 to indicate KFENCE is disabled. */ >         WRITE_ONCE(kfence_enabled, false); > Hmm... I found KFENCE_WARN_ON() may be called when sample_interval==0. (e.g., kfence_guarded_free()) So it's better to add a bool. diff --git a/mm/kfence/core.c b/mm/kfence/core.c index ae69b2a113a4..c729be0207e8 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -38,14 +38,17 @@ #define KFENCE_WARN_ON(cond) \ ({ \ const bool __cond = WARN_ON(cond); \ - if (unlikely(__cond)) \ + if (unlikely(__cond)) { \ WRITE_ONCE(kfence_enabled, false); \ + disabled_by_warn = true; \ + } \ __cond; \ }) /* === Data ================================================================= */ static bool kfence_enabled __read_mostly; +static bool disabled_by_warn __read_mostly; unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE_INTERVAL; EXPORT_SYMBOL_GPL(kfence_sample_interval); /* Export for test modules. */ @@ -70,7 +73,7 @@ static int param_set_sample_interval(const char *val, const struct kernel_param *((unsigned long *)kp->arg) = num; if (num && !READ_ONCE(kfence_enabled) && system_state != SYSTEM_BOOTING) - return kfence_enable_late(); + return disabled_by_warn ? -EINVAL : kfence_enable_late(); return 0; }