RE: [PATCH] module: Add CONFIG_MODULE_LOAD_IN_SEQUENCE option

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
Date: Wed, 11 Oct 2023 01:57:58 +0000	[thread overview]
Message-ID: <49a0f7c36d8d41abb1204b0780667fe0@quicinc.com> (raw)
In-Reply-To: <ZSXR7PJmXZpZxQ2R@bombadil.infradead.org>

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 module 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 can always get right function/line number from addr2line.
But module address keeps change across rebooting, in first booting, it might 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/syzkaller/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 
> are not in fixed sequence as expected.
> 
> Add the option to make sure modules are in fixed sequence across reboot.
> 
> Signed-off-by: Joey Jiao <quic_jiangenj@quicinc.com>
> ---
>  kernel/module/Kconfig | 11 +++++++++++  kernel/module/main.c  |  6 
> ++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig index 
> 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
>  
> +config MODULE_LOAD_IN_SEQUENCE
> +	bool "Load module in sequence"
> +	default n
> +	help
> +	  By default, modules are loaded in random sequence depending on when modprobe
> +	  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 syzkaller 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 
> 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