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;
MIME-Version: 1.0
References: <20220424105949.50016-1-huangshaobo6@huawei.com> <20220425022456.44300-1-huangshaobo6@huawei.com>
In-Reply-To: <20220425022456.44300-1-huangshaobo6@huawei.com>
From:   Marco Elver <elver@google.com>
Date:   Mon, 25 Apr 2022 10:15:43 +0200
Message-ID: <CANpmjNO=Qo_wnZ2CH=GqPzyUwQ3jGq_Z9FNQt+Sc_=1ZMV2PfQ@mail.gmail.com>
Subject: Re: [PATCH v3] kfence: enable check kfence canary on panic via boot param
To:     Shaobo Huang <huangshaobo6@huawei.com>
Cc:     glider@google.com, dvyukov@google.com, akpm@linux-foundation.org,
        kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, young.liuyang@huawei.com,
        zengweilin@huawei.com, chenzefeng2@huawei.com,
        nixiaoming@huawei.com, wangbing6@huawei.com,
        wangfangpeng1@huawei.com, zhongjubin@huawei.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 25 Apr 2022 at 04:25, 'Shaobo Huang' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> From: huangshaobo <huangshaobo6@huawei.com>
>
> Out-of-bounds accesses that aren't caught by a guard page will result
> in corruption of canary memory. In pathological cases, where an object
> has certain alignment requirements, an out-of-bounds access might
> never be caught by the guard page. Such corruptions, however, are only
> detected on kfree() normally. If the bug causes the kernel to panic
> before kfree(), KFENCE has no opportunity to report the issue. Such
> corruptions may also indicate failing memory or other faults.
>
> To provide some more information in such cases, add the option to
> check canary bytes on panic. This might help narrow the search for the
> panic cause; but, due to only having the allocation stack trace, such
> reports are difficult to use to diagnose an issue alone. In most
> cases, such reports are inactionable, and is therefore an opt-in
> feature (disabled by default).
>
> Suggested-by: chenzefeng <chenzefeng2@huawei.com>
> Signed-off-by: huangshaobo <huangshaobo6@huawei.com>

I missed one minor issue below (__read_mostly for param), but with
that in place:

Reviewed-by: Marco Elver <elver@google.com>

> ---
> v3:
> - use Marco's description replace the commit message
> - keep these includes sorted alphabetically
> - "in panic" replaced with "on panic" in title and comments
> - Blank line between /* === ... */ and function.
> v2:
> - it is only detected in panic.
> - it is disabled by default.
> - can only be enabled via boot parameter.
> - the code is moved to the specified partition.
>   https://lore.kernel.org/all/20220424105949.50016-1-huangshaobo6@huawei.com/
> v1:
>   https://lore.kernel.org/all/20220420104927.59056-1-huangshaobo6@huawei.com/
> Thanks again Marco for the suggestion.
> ---
>  mm/kfence/core.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 9b2b5f56f4ae..06232d51e021 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -21,6 +21,8 @@
>  #include <linux/log2.h>
>  #include <linux/memblock.h>
>  #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
> +#include <linux/panic_notifier.h>
>  #include <linux/random.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched/clock.h>
> @@ -99,6 +101,10 @@ module_param_named(skip_covered_thresh, kfence_skip_covered_thresh, ulong, 0644)
>  static bool kfence_deferrable __read_mostly = IS_ENABLED(CONFIG_KFENCE_DEFERRABLE);
>  module_param_named(deferrable, kfence_deferrable, bool, 0444);
>
> +/* If true, check all canary bytes on panic. */
> +static bool kfence_check_on_panic;

This should be __read_mostly, like the other params.

Sorry for noticing this late.

> +module_param_named(check_on_panic, kfence_check_on_panic, bool, 0444);
> +

Thanks,
-- Marco