Re: [PATCH v5] arm64/module: Use text-poke API for late relocations.

[Will Deacon <will@kernel.org>]

Hey Dylan,

On Fri, May 30, 2025 at 05:11:00PM -0700, Dylan Hatch wrote:
> On Fri, May 30, 2025 at 7:13 AM Will Deacon <will@kernel.org> wrote:
> >
> > and this would be:
> >
> >         WRITE_PLACE(place, cpu_to_le32(insn), me);
> >
> 
> I'm seeing this part give a build error:
> 
> arch/arm64/kernel/module.c:158:2: error: cannot take the address of an
> rvalue of type '__le32' (aka 'unsigned int')
>   158 |         WRITE_PLACE(place, cpu_to_le32(insn), me);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/arm64/kernel/module.c:56:28: note: expanded from macro 'WRITE_PLACE'
>    56 |                 aarch64_insn_copy(place, &(val),
> sizeof(*place));       \
>       |                                          ^ ~~~
> 
> I can't think of a clean way to get around this and still keep a
> combined write helper. Setting an intermediate __le32 in the
> reloc_insn_* functions would work but we were trying to avoid that.
> Setting an intermediate value inside WRITE_PLACE would also work but
> then (I think) it won't work for the data relocations because we'd be
> converting a signed into unsigned value. Making WRITE_PLACE a function
> instead of a macro also fixes the rvalue problem but then the args
> 'place' and 'val' have to be of a fixed type so we can't do the
> typecasting on 'place' and 'val' has the same signed/unsigned value
> problem.
> 
> Do you have a suggestion here? In the meantime I can send a v6 that
> uses an intermediate __le32 for the instruction relocations.

Sorry for the slow reply -- I see you already sent a v6. I think we
could add a temporary in the macro. Diff below (on top of your v6). WDYT?

Will

--->8

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 862f6d50ab00..40148d2725ce 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -50,10 +50,12 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, __le32 *place, u64 val)
 }
 
 #define WRITE_PLACE(place, val, mod) do {				\
+	__typeof__(val) __val = (val);					\
+									\
 	if (mod->state == MODULE_STATE_UNFORMED)			\
-		*(place) = val;						\
+		*(place) = __val;					\
 	else								\
-		aarch64_insn_copy(place, &(val), sizeof(*place));	\
+		aarch64_insn_copy(place, &(__val), sizeof(*place));	\
 } while (0)
 
 static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len,
@@ -128,7 +130,6 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
 	u64 imm;
 	s64 sval;
 	u32 insn = le32_to_cpu(*place);
-	__le32 le_insn;
 
 	sval = do_reloc(op, place, val);
 	imm = sval >> lsb;
@@ -156,8 +157,7 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
 
 	/* Update the instruction with the new encoding. */
 	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
-	le_insn = cpu_to_le32(insn);
-	WRITE_PLACE(place, le_insn, me);
+	WRITE_PLACE(place, cpu_to_le32(insn), me);
 
 	if (imm > U16_MAX)
 		return -ERANGE;
@@ -172,7 +172,6 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
 	u64 imm, imm_mask;
 	s64 sval;
 	u32 insn = le32_to_cpu(*place);
-	__le32 le_insn;
 
 	/* Calculate the relocation value. */
 	sval = do_reloc(op, place, val);
@@ -184,8 +183,7 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
 
 	/* Update the instruction's immediate field. */
 	insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
-	le_insn = cpu_to_le32(insn);
-	WRITE_PLACE(place, le_insn, me);
+	WRITE_PLACE(place, cpu_to_le32(insn), me);
 
 	/*
 	 * Extract the upper value bits (including the sign bit) and
@@ -207,7 +205,6 @@ static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
 			   __le32 *place, u64 val, struct module *me)
 {
 	u32 insn;
-	__le32 le_insn;
 
 	if (!is_forbidden_offset_for_adrp(place))
 		return reloc_insn_imm(RELOC_OP_PAGE, place, val, 12, 21,
@@ -227,8 +224,7 @@ static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
 						   AARCH64_INSN_BRANCH_NOLINK);
 	}
 
-	le_insn = cpu_to_le32(insn);
-	WRITE_PLACE(place, le_insn, me);
+	WRITE_PLACE(place, cpu_to_le32(insn), me);
 	return 0;
 }