[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ede19a0d-4070-568f-becb-13872a7191a5@huawei.com>
Date: Wed, 4 Sep 2024 17:19:47 +0800
From: Jinjie Ruan <ruanjinjie@...wei.com>
To: Thomas Gleixner <tglx@...utronix.de>, <linux-kernel@...r.kernel.org>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>, Christophe Leroy
<christophe.leroy@...roup.eu>, Josh Poimboeuf <jpoimboe@...nel.org>
Subject: Re: [PATCH v2] static_call: Handle module init failure correctly in
static_call_del_module()
On 2024/9/4 17:09, Thomas Gleixner wrote:
> Module insertion invokes static_call_add_module() to initialize the static
> calls in a module. static_call_add_module() invokes __static_call_init(),
> which allocates a struct static_call_mod to either encapsulate the built-in
> static call sites of the associated key into it so further modules can be
> added or to append the module to the module chain.
>
> If that allocation fails the function returns with an error code and the
> module core invokes static_call_del_module() to clean up eventually added
> static_call_mod entries.
>
> This works correctly, when all keys used by the module were converted over
> to a module chain before the failure. If not then static_call_del_module()
> causes a #GP as it blindly assumes that key::mods points to a valid struct
> static_call_mod.
>
> The problem is that key::mods is not a individual struct member of struct
> static_call_key, it's part of a union to save space:
>
> union {
> /* bit 0: 0 = mods, 1 = sites */
> unsigned long type;
> struct static_call_mod *mods;
> struct static_call_site *sites;
> };
>
> key::sites is a pointer to the list of built-in usage sites of the static
> call. The type of the pointer is differentiated by bit 0. A mods pointer
> has the bit clear, the sites pointer has the bit set.
>
> As static_call_del_module() blidly assumes that the pointer is a valid
> static_call_mod type, it fails to check for this failure case and
> dereferences the pointer to the list of built-in call sites, which is
> obviously bogus.
>
> Cure it by checking whether the key has a sites or a mods pointer.
>
> If it's a sites pointer then the key is not to be touched. As the sites are
> walked in the same order as in __static_call_init() the site walk can be
> terminated because all subsequent sites have not been touched by the init
> code due to the error exit.
>
> If it was converted before the allocation fail, then the inner loop which
> searches for a module match will find nothing.
>
> A fail in the second allocation in __static_call_init() is harmless and
> does not require special treatment. The first allocation succeeded and
> converted the key to a module chain. That first entry has mod::mod == NULL
> and mod::next == NULL, so the inner loop of static_call_del_module() will
> neither find a module match nor a module chain. The next site in the walk
> was either already converted, but can't match the module, or it will exit
> the outer loop because it has a static_call_site pointer and not a
> static_call_mod pointer.
>
> Fixes: 9183c3f9ed71 ("static_call: Add inline static call infrastructure")
> Reported-by: Jinjie Ruan <ruanjinjie@...wei.com>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Closes: https://lore.kernel.org/all/20230915082126.4187913-1-ruanjinjie@huawei.com
> ---
> V2: Use static_call_key_has_mods() instead
> ---
> kernel/static_call_inline.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> --- a/kernel/static_call_inline.c
> +++ b/kernel/static_call_inline.c
> @@ -411,6 +411,17 @@ static void static_call_del_module(struc
>
> for (site = start; site < stop; site++) {
> key = static_call_key(site);
> +
> + /*
> + * If the key was not updated due to a memory allocation
> + * failure in __static_call_init() then treating key::sites
> + * as key::mods in the code below would cause random memory
> + * access and #GP. In that case all subsequent sites have
> + * not been touched either, so stop iterating.
> + */
> + if (!static_call_key_has_mods(key))
> + break;
> +
Tested-by: Jinjie Ruan <ruanjinjie@...wei.com>
> if (key == prev_key)
> continue;
>
Powered by blists - more mailing lists