linux-kernel - Re: [PATCH v2 01/10] lib/min_heap: Introduce non-inline versions of min heap API functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdVO5DPuD9HYWBFqKDHphx7+0BEhreUxtVC40A=8p6VAhQ@mail.gmail.com>
Date: Tue, 26 Nov 2024 14:27:09 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Kuan-Wei Chiu <visitorckw@...il.com>
Cc: colyli@...e.de, kent.overstreet@...ux.dev, msakai@...hat.com, 
	corbet@....net, peterz@...radead.org, mingo@...hat.com, acme@...nel.org, 
	namhyung@...nel.org, akpm@...ux-foundation.org, mark.rutland@....com, 
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com, 
	adrian.hunter@...el.com, kan.liang@...ux.intel.com, willy@...radead.org, 
	jserv@...s.ncku.edu.tw, linux-kernel@...r.kernel.org, 
	linux-bcache@...r.kernel.org, dm-devel@...ts.linux.dev, 
	linux-bcachefs@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	linux-doc@...r.kernel.org, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v2 01/10] lib/min_heap: Introduce non-inline versions of
 min heap API functions

Hi Kuan-Wei,

On Sun, Oct 20, 2024 at 6:02 AM Kuan-Wei Chiu <visitorckw@...il.com> wrote:
> All current min heap API functions are marked with '__always_inline'.
> However, as the number of users increases, inlining these functions
> everywhere leads to a increase in kernel size.
>
> In performance-critical paths, such as when perf events are enabled and
> min heap functions are called on every context switch, it is important
> to retain the inline versions for optimal performance. To balance this,
> the original inline functions are kept, and additional non-inline
> versions of the functions have been added in lib/min_heap.c.
>
> Link: https://lore.kernel.org/20240522161048.8d8bbc7b153b4ecd92c50666@linux-foundation.org
> Suggested-by: Andrew Morton <akpm@...ux-foundation.org>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@...il.com>

Thanks for your patch, which is now commit 92a8b224b833e82d
("lib/min_heap: introduce non-inline versions of min heap API
functions") upstream.

> --- a/include/linux/min_heap.h
> +++ b/include/linux/min_heap.h

> @@ -50,33 +50,33 @@ void __min_heap_init(min_heap_char *heap, void *data, int size)
>                 heap->data = heap->preallocated;
>  }
>
> -#define min_heap_init(_heap, _data, _size)     \
> -       __min_heap_init((min_heap_char *)_heap, _data, _size)
> +#define min_heap_init_inline(_heap, _data, _size)      \
> +       __min_heap_init_inline((min_heap_char *)_heap, _data, _size)

Casting macro parameters without any further checks prevents the
compiler from detecting silly mistakes.  Would it be possible to
add safety-nets here and below, using e.g. container_of() or typeof()
checks?

> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -777,3 +777,6 @@ config POLYNOMIAL
>
>  config FIRMWARE_TABLE
>         bool
> +
> +config MIN_HEAP
> +       bool

Perhaps tristate? See also below.

> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2279,6 +2279,7 @@ config TEST_LIST_SORT
>  config TEST_MIN_HEAP
>         tristate "Min heap test"
>         depends on DEBUG_KERNEL || m
> +       select MIN_HEAP

Ideally, tests should not select functionality, to prevent increasing the
attack vector by merely enabling (modular) tests.

In this particular case, just using "depends on MIN_HEAP" is not an
option, as MIN_HEAP is not user-visible, and thus cannot be enabled
by the user on its own.  However, making MIN_HEAP tristate could be
a first step for the modular case.

The builtin case is harder to fix, as e.g.

        depends on MIN_HEAP || COMPILE_TEST
        select MIN_HEAP if COMPILE_TEST

would still trigger a recursive dependency error.

Alternatively, the test could just keep on using the inline variants,
unless CONFIG_MIN_HEAP=y? Or event test both for the latter?

>         help
>           Enable this to turn on min heap function tests. This test is
>           executed only once during system boot (so affects only boot time),

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds