linux-kernel - Re: [PATCH v2 19/28] LoongArch: ftrace: Use RCU in all users of __module_text

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] [day] [month] [year] [list]

Message-ID: <20241227121946.1643decf@gandalf.local.home>
Date: Fri, 27 Dec 2024 12:19:46 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org, Daniel
 Gomez <da.gomez@...sung.com>, Luis Chamberlain <mcgrof@...nel.org>, "Paul E
 . McKenney" <paulmck@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
 Petr Pavlu <petr.pavlu@...e.com>, Sami Tolvanen <samitolvanen@...gle.com>,
 Thomas Gleixner <tglx@...utronix.de>, Huacai Chen <chenhuacai@...nel.org>,
 Mark Rutland <mark.rutland@....com>, Masami Hiramatsu
 <mhiramat@...nel.org>, WANG Xuerui <kernel@...0n.name>,
 linux-trace-kernel@...r.kernel.org, loongarch@...ts.linux.dev
Subject: Re: [PATCH v2 19/28] LoongArch: ftrace: Use RCU in all users of
 __module_text_address().

On Fri, 20 Dec 2024 18:41:33 +0100
Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:

> --- a/arch/loongarch/kernel/ftrace_dyn.c
> +++ b/arch/loongarch/kernel/ftrace_dyn.c
> @@ -85,14 +85,13 @@ static bool ftrace_find_callable_addr(struct dyn_ftrace *rec, struct module *mod
>  	 * dealing with an out-of-range condition, we can assume it
>  	 * is due to a module being loaded far away from the kernel.
>  	 *
> -	 * NOTE: __module_text_address() must be called with preemption
> -	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
> +	 * NOTE: __module_text_address() must be called within a RCU read
> +	 * section, but we can rely on ftrace_lock to ensure that 'mod'
>  	 * retains its validity throughout the remainder of this code.
>  	 */
>  	if (!mod) {
> -		preempt_disable();
> +		guard(rcu)();
>  		mod = __module_text_address(pc);
> -		preempt_enable();
>  	}
>  
>  	if (WARN_ON(!mod))
> -- 

I personally dislike swapping one line of protection for the guard() code.

Although, I do think scoped_guard() can work.

That is:

	if (!mod) {
		read_rcu_lock();
		mod = __module_text_address(pc);
		read_rcu_unlock();
	}

Is easier to understand than:

	if (!mod) {
		guard(rcu)();
		mod = __module_text_address(pc);
	}

Because it makes me wonder, why use a guard() for a one liner?

But, when I saw your other patch, if we had:

	if (!mod) {
		scoped_guard(rcu)()
			mod = __module_text_address(pc);
	}

To me, hat looks much better than the guard() as it is obvious to what the
code is protecting. Even though, I still prefer the explicit, lock/unlock.
Maybe, just because I'm more used to it.

IMHO, guard() is for complex functions that are error prone. A single line
is not something that is error prone (unless you don't match the lock and
unlock properly, but that's pretty obvious when that happens).

But this is just my own opinion.

-- Steve