Date: Tue, 27 May 2025 16:46:56 +0200
From: Petr Mladek <pmladek@suse.com>
To: Dylan Hatch <dylanbhatch@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
	Joe Lawrence <joe.lawrence@redhat.com>, Song Liu <song@kernel.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	"Mike Rapoport (Microsoft)" <rppt@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dan Carpenter <dan.carpenter@linaro.org>,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	live-patching@vger.kernel.org,
	Roman Gushchin <roman.gushchin@linux.dev>,
	Toshiyuki Sato <fj6611ie@aa.jp.fujitsu.com>
Subject: Re: [PATCH v4 1/2] livepatch, x86/module: Generalize late module
 relocation locking.
Message-ID: <aDXQYMcLle2E_b2d@pathway.suse.cz>
References: <20250522205205.3408764-1-dylanbhatch@google.com>
 <20250522205205.3408764-2-dylanbhatch@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20250522205205.3408764-2-dylanbhatch@google.com>

On Thu 2025-05-22 20:52:04, Dylan Hatch wrote:
> Late module relocations are an issue on any arch that supports
> livepatch, so move the text_mutex locking to the livepatch core code.
> 
> Signed-off-by: Dylan Hatch <dylanbhatch@google.com>
> Acked-by: Song Liu <song@kernel.org>
> ---
>  arch/x86/kernel/module.c |  8 ++------
>  kernel/livepatch/core.c  | 18 +++++++++++++-----
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -197,18 +197,14 @@ static int write_relocate_add(Elf64_Shdr *sechdrs,
>  	bool early = me->state == MODULE_STATE_UNFORMED;
>  	void *(*write)(void *, const void *, size_t) = memcpy;
>  
> -	if (!early) {
> +	if (!early)
>  		write = text_poke;
> -		mutex_lock(&text_mutex);
> -	}
>  
>  	ret = __write_relocate_add(sechdrs, strtab, symindex, relsec, me,
>  				   write, apply);
>  
> -	if (!early) {
> +	if (!early)
>  		text_poke_sync();
> -		mutex_unlock(&text_mutex);
> -	}
>  
>  	return ret;
>  }
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 0e73fac55f8eb..9968441f73510 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -319,12 +320,19 @@ static int klp_write_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
>  					  sec, sec_objname);
>  		if (ret)
>  			return ret;
> -
> -		return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
>  	}
>  
> -	clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> -	return 0;
> +	if (!early)
> +		mutex_lock(&text_mutex);

I understand why you do this but it opens some questions.

As this patch suggests, the "text_mutex" has been used to
sychronize apply_relocate_add() only on x86_64 so far.

s390x seems to rely on "s390_kernel_write_lock" taken by:

  + apply_relocate_add()
    + s390_kernel_write()
      + __s390_kernel_write()

And powerpc seems to rely on "pte" locking taken by

  + apply_relocate_add()
    + patch_instruction()
      + patch_mem()
	+ __do_patch_mem_mm()
	  + get_locked_pte()

I see two possibilities:

  1. Either this change makes a false feeling that "text_mutex"
     sychronizes apply_relocate_add() on all architextures.

     This does not seems to be the case on, for example, s390
     and powerpc.

     => The code is misleading and could lead to troubles.


   2. Or it actually provides some sychronization on all
      architectures, for example, against kprobe code.

      In this case, it might actually fix an existing race.
      It should be described in the commit message
      and nominated for backporting to stable.


I am sorry if this has already been discussed. But I have been
in Cc only for v3 and v4. And there is no changelog in
the cover letter.

> +
> +	if (apply)
> +		ret = apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> +	else
> +		clear_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
> +
> +	if (!early)
> +		mutex_unlock(&text_mutex);
> +	return ret;
>  }

Best Regards,
Petr