Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
From:   "Joey Jiao (QUIC)" <quic_jiangenj@quicinc.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
CC:     "linux-modules@vger.kernel.org" <linux-modules@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option
Thread-Topic: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option
Thread-Index: AQHZ+m0OO8dn2vGceUm2vHhzod0cabBEFOkA//+7qJA=
Date:   Wed, 11 Oct 2023 01:57:58 +0000
Message-ID: <49a0f7c36d8d41abb1204b0780667fe0@quicinc.com>
References: <20231009045636.4143-1-quic_jiangenj@quicinc.com>
 <ZSXR7PJmXZpZxQ2R@bombadil.infradead.org>
In-Reply-To: <ZSXR7PJmXZpZxQ2R@bombadil.infradead.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

Hi Luis,

> How is ignoring an error ensuring ordering?
It's not ignoring an error ensuring ordering.
I think It's because `schedule_work(&init_free_wq);` changes when the free_=
wq will happen.
If sometimes freeing init for module A happens before loading another modul=
e B, that memory is freed. B might be loaded into that freed address.
If sometimes freeing init for module A happens after loading another module=
 B, B might be loaded into another memory address.
The change is just to disable the schedule_work.

> Why are you making this only now be called with this new kconfig option?
This sequence loading is especially helpful for syzkaller coverage decoding=
.
When kaslr is disabled, address inside core kernel is fixed, so syzkaller c=
an always get right function/line number from addr2line.
But module address keeps change across rebooting, in first booting, it migh=
t be loaded at X1, and at X2 after reboot, and at X3 after another reboot.
In this way, syzkaller just can't decode correctly for module address. And =
syzkaller currently uses PC and branch info for coverage guided things.

There was a discussion previously here https://groups.google.com/g/syzkalle=
r/c/1Pnm_BjrZO8/m/WOyAKx8ZAgAJ for modprobe.

THX
Joey
-----Original Message-----
From: Luis Chamberlain <mcgrof@infradead.org> On Behalf Of Luis Chamberlain
Sent: Wednesday, October 11, 2023 6:36 AM
To: Joey Jiao (QUIC) <quic_jiangenj@quicinc.com>
Cc: linux-modules@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

On Mon, Oct 09, 2023 at 10:26:35AM +0530, Joey Jiao wrote:
> When modprobe cmds are executed one by one, the final loaded modules=20
> are not in fixed sequence as expected.
>=20
> Add the option to make sure modules are in fixed sequence across reboot.
>=20
> Signed-off-by: Joey Jiao <quic_jiangenj@quicinc.com>
> ---
>  kernel/module/Kconfig | 11 +++++++++++  kernel/module/main.c  |  6=20
> ++++++
>  2 files changed, 17 insertions(+)
>=20
> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig index=20
> 33a2e991f608..b45a45f31d6d 100644
> --- a/kernel/module/Kconfig
> +++ b/kernel/module/Kconfig
> @@ -389,4 +389,15 @@ config MODULES_TREE_LOOKUP
>  	def_bool y
>  	depends on PERF_EVENTS || TRACING || CFI_CLANG
> =20
> +config MODULE_LOAD_IN_SEQUENCE
> +	bool "Load module in sequence"
> +	default n
> +	help
> +	  By default, modules are loaded in random sequence depending on when m=
odprobe
> +	  is executed.
> +
> +	  This option allows modules to be loaded in sequence if modprobe cmds =
are
> +	  executed one by one in sequence. This option is helpful during syzkal=
ler fuzzing
> +	  to make sure module is loaded into fixed address across device reboot=
.
> +
>  endif # MODULES
> diff --git a/kernel/module/main.c b/kernel/module/main.c index=20
> 98fedfdb8db5..587fd84083ae 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2593,11 +2593,17 @@ static noinline int do_init_module(struct module =
*mod)
>  	 * be cleaned up needs to sync with the queued work - ie
>  	 * rcu_barrier()
>  	 */
> +#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
> +	llist_add(&freeinit->node, &init_free_list); #else
>  	if (llist_add(&freeinit->node, &init_free_list))
>  		schedule_work(&init_free_wq);
> +#endif

How is ignoring an error ensuring ordering?

>  	mutex_unlock(&module_mutex);
> +#ifdef CONFIG_MODULE_LOAD_IN_SEQUENCE
>  	wake_up_all(&module_wq);
> +#endif

Why are you making this only now be called with this new kconfig option?

  Luis