* [PATCH v5] arm64/module: Use text-poke API for late relocations.
@ 2025-05-30 0:00 Dylan Hatch
2025-05-30 14:13 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: Dylan Hatch @ 2025-05-30 0:00 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon, Ard Biesheuvel, Sami Tolvanen,
Geert Uytterhoeven, Song Liu
Cc: linux-arm-kernel, linux-kernel, Dylan Hatch, Roman Gushchin,
Toshiyuki Sato
To enable late module patching, livepatch modules need to be able to
apply some of their relocations well after being loaded. In this
scenario however, the livepatch module text and data is already RX-only,
so special treatment is needed to make the late relocations possible. To
do this, use the text-poking API for these late relocations.
This patch is partially based off commit 88fc078a7a8f6 ("x86/module: Use
text_poke() for late relocations").
Signed-off-by: Dylan Hatch <dylanbhatch@google.com>
Acked-by: Song Liu <song@kernel.org>
---
arch/arm64/kernel/module.c | 110 ++++++++++++++++++++++---------------
1 file changed, 66 insertions(+), 44 deletions(-)
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 06bb680bfe975..93e6d63074afe 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -23,6 +23,7 @@
#include <asm/insn.h>
#include <asm/scs.h>
#include <asm/sections.h>
+#include <asm/text-patching.h>
enum aarch64_reloc_op {
RELOC_OP_NONE,
@@ -48,7 +49,26 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, __le32 *place, u64 val)
return 0;
}
-static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
+static void write_data(void *place, s64 *sval, size_t len, struct module *me)
+{
+ if (me->state == MODULE_STATE_UNFORMED)
+ memcpy(place, sval, len);
+ else
+ aarch64_insn_copy(place, sval, len);
+}
+
+static void write_insn(__le32 *place, u32 insn, struct module *me)
+{
+ __le32 le = cpu_to_le32(insn);
+
+ if (me->state == MODULE_STATE_UNFORMED)
+ *place = le;
+ else
+ aarch64_insn_copy(place, &le, sizeof(le));
+}
+
+static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len,
+ struct module *me)
{
s64 sval = do_reloc(op, place, val);
@@ -66,7 +86,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
switch (len) {
case 16:
- *(s16 *)place = sval;
+ write_data(place, &sval, sizeof(s16), me);
switch (op) {
case RELOC_OP_ABS:
if (sval < 0 || sval > U16_MAX)
@@ -82,7 +102,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
}
break;
case 32:
- *(s32 *)place = sval;
+ write_data(place, &sval, sizeof(s32), me);
switch (op) {
case RELOC_OP_ABS:
if (sval < 0 || sval > U32_MAX)
@@ -98,7 +118,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
}
break;
case 64:
- *(s64 *)place = sval;
+ write_data(place, &sval, sizeof(s64), me);
break;
default:
pr_err("Invalid length (%d) for data relocation\n", len);
@@ -113,7 +133,8 @@ enum aarch64_insn_movw_imm_type {
};
static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
- int lsb, enum aarch64_insn_movw_imm_type imm_type)
+ int lsb, enum aarch64_insn_movw_imm_type imm_type,
+ struct module *me)
{
u64 imm;
s64 sval;
@@ -145,7 +166,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);
- *place = cpu_to_le32(insn);
+ write_insn(place, insn, me);
if (imm > U16_MAX)
return -ERANGE;
@@ -154,7 +175,8 @@ static int reloc_insn_movw(enum aarch64_reloc_op op, __le32 *place, u64 val,
}
static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
- int lsb, int len, enum aarch64_insn_imm_type imm_type)
+ int lsb, int len, enum aarch64_insn_imm_type imm_type,
+ struct module *me)
{
u64 imm, imm_mask;
s64 sval;
@@ -170,7 +192,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);
- *place = cpu_to_le32(insn);
+ write_insn(place, insn, me);
/*
* Extract the upper value bits (including the sign bit) and
@@ -189,17 +211,17 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, __le32 *place, u64 val,
}
static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
- __le32 *place, u64 val)
+ __le32 *place, u64 val, struct module *me)
{
u32 insn;
if (!is_forbidden_offset_for_adrp(place))
return reloc_insn_imm(RELOC_OP_PAGE, place, val, 12, 21,
- AARCH64_INSN_IMM_ADR);
+ AARCH64_INSN_IMM_ADR, me);
/* patch ADRP to ADR if it is in range */
if (!reloc_insn_imm(RELOC_OP_PREL, place, val & ~0xfff, 0, 21,
- AARCH64_INSN_IMM_ADR)) {
+ AARCH64_INSN_IMM_ADR, me)) {
insn = le32_to_cpu(*place);
insn &= ~BIT(31);
} else {
@@ -211,7 +233,7 @@ static int reloc_insn_adrp(struct module *mod, Elf64_Shdr *sechdrs,
AARCH64_INSN_BRANCH_NOLINK);
}
- *place = cpu_to_le32(insn);
+ write_insn(place, insn, me);
return 0;
}
@@ -255,23 +277,23 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
/* Data relocations. */
case R_AARCH64_ABS64:
overflow_check = false;
- ovf = reloc_data(RELOC_OP_ABS, loc, val, 64);
+ ovf = reloc_data(RELOC_OP_ABS, loc, val, 64, me);
break;
case R_AARCH64_ABS32:
- ovf = reloc_data(RELOC_OP_ABS, loc, val, 32);
+ ovf = reloc_data(RELOC_OP_ABS, loc, val, 32, me);
break;
case R_AARCH64_ABS16:
- ovf = reloc_data(RELOC_OP_ABS, loc, val, 16);
+ ovf = reloc_data(RELOC_OP_ABS, loc, val, 16, me);
break;
case R_AARCH64_PREL64:
overflow_check = false;
- ovf = reloc_data(RELOC_OP_PREL, loc, val, 64);
+ ovf = reloc_data(RELOC_OP_PREL, loc, val, 64, me);
break;
case R_AARCH64_PREL32:
- ovf = reloc_data(RELOC_OP_PREL, loc, val, 32);
+ ovf = reloc_data(RELOC_OP_PREL, loc, val, 32, me);
break;
case R_AARCH64_PREL16:
- ovf = reloc_data(RELOC_OP_PREL, loc, val, 16);
+ ovf = reloc_data(RELOC_OP_PREL, loc, val, 16, me);
break;
/* MOVW instruction relocations. */
@@ -280,88 +302,88 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
fallthrough;
case R_AARCH64_MOVW_UABS_G0:
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 0,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_UABS_G1_NC:
overflow_check = false;
fallthrough;
case R_AARCH64_MOVW_UABS_G1:
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 16,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_UABS_G2_NC:
overflow_check = false;
fallthrough;
case R_AARCH64_MOVW_UABS_G2:
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 32,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_UABS_G3:
/* We're using the top bits so we can't overflow. */
overflow_check = false;
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 48,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_SABS_G0:
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 0,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
case R_AARCH64_MOVW_SABS_G1:
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 16,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
case R_AARCH64_MOVW_SABS_G2:
ovf = reloc_insn_movw(RELOC_OP_ABS, loc, val, 32,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
case R_AARCH64_MOVW_PREL_G0_NC:
overflow_check = false;
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 0,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_PREL_G0:
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 0,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
case R_AARCH64_MOVW_PREL_G1_NC:
overflow_check = false;
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 16,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_PREL_G1:
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 16,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
case R_AARCH64_MOVW_PREL_G2_NC:
overflow_check = false;
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 32,
- AARCH64_INSN_IMM_MOVKZ);
+ AARCH64_INSN_IMM_MOVKZ, me);
break;
case R_AARCH64_MOVW_PREL_G2:
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 32,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
case R_AARCH64_MOVW_PREL_G3:
/* We're using the top bits so we can't overflow. */
overflow_check = false;
ovf = reloc_insn_movw(RELOC_OP_PREL, loc, val, 48,
- AARCH64_INSN_IMM_MOVNZ);
+ AARCH64_INSN_IMM_MOVNZ, me);
break;
/* Immediate instruction relocations. */
case R_AARCH64_LD_PREL_LO19:
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 19,
- AARCH64_INSN_IMM_19);
+ AARCH64_INSN_IMM_19, me);
break;
case R_AARCH64_ADR_PREL_LO21:
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 0, 21,
- AARCH64_INSN_IMM_ADR);
+ AARCH64_INSN_IMM_ADR, me);
break;
case R_AARCH64_ADR_PREL_PG_HI21_NC:
overflow_check = false;
fallthrough;
case R_AARCH64_ADR_PREL_PG_HI21:
- ovf = reloc_insn_adrp(me, sechdrs, loc, val);
+ ovf = reloc_insn_adrp(me, sechdrs, loc, val, me);
if (ovf && ovf != -ERANGE)
return ovf;
break;
@@ -369,46 +391,46 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
case R_AARCH64_LDST8_ABS_LO12_NC:
overflow_check = false;
ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 0, 12,
- AARCH64_INSN_IMM_12);
+ AARCH64_INSN_IMM_12, me);
break;
case R_AARCH64_LDST16_ABS_LO12_NC:
overflow_check = false;
ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 1, 11,
- AARCH64_INSN_IMM_12);
+ AARCH64_INSN_IMM_12, me);
break;
case R_AARCH64_LDST32_ABS_LO12_NC:
overflow_check = false;
ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 2, 10,
- AARCH64_INSN_IMM_12);
+ AARCH64_INSN_IMM_12, me);
break;
case R_AARCH64_LDST64_ABS_LO12_NC:
overflow_check = false;
ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 3, 9,
- AARCH64_INSN_IMM_12);
+ AARCH64_INSN_IMM_12, me);
break;
case R_AARCH64_LDST128_ABS_LO12_NC:
overflow_check = false;
ovf = reloc_insn_imm(RELOC_OP_ABS, loc, val, 4, 8,
- AARCH64_INSN_IMM_12);
+ AARCH64_INSN_IMM_12, me);
break;
case R_AARCH64_TSTBR14:
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 14,
- AARCH64_INSN_IMM_14);
+ AARCH64_INSN_IMM_14, me);
break;
case R_AARCH64_CONDBR19:
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 19,
- AARCH64_INSN_IMM_19);
+ AARCH64_INSN_IMM_19, me);
break;
case R_AARCH64_JUMP26:
case R_AARCH64_CALL26:
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2, 26,
- AARCH64_INSN_IMM_26);
+ AARCH64_INSN_IMM_26, me);
if (ovf == -ERANGE) {
val = module_emit_plt_entry(me, sechdrs, loc, &rel[i], sym);
if (!val)
return -ENOEXEC;
ovf = reloc_insn_imm(RELOC_OP_PREL, loc, val, 2,
- 26, AARCH64_INSN_IMM_26);
+ 26, AARCH64_INSN_IMM_26, me);
}
break;
--
2.49.0.1204.g71687c7c1d-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5] arm64/module: Use text-poke API for late relocations.
2025-05-30 0:00 [PATCH v5] arm64/module: Use text-poke API for late relocations Dylan Hatch
@ 2025-05-30 14:13 ` Will Deacon
2025-05-31 0:11 ` Dylan Hatch
0 siblings, 1 reply; 5+ messages in thread
From: Will Deacon @ 2025-05-30 14:13 UTC (permalink / raw)
To: Dylan Hatch
Cc: Catalin Marinas, Ard Biesheuvel, Sami Tolvanen,
Geert Uytterhoeven, Song Liu, linux-arm-kernel, linux-kernel,
Roman Gushchin, Toshiyuki Sato
On Fri, May 30, 2025 at 12:00:44AM +0000, Dylan Hatch wrote:
> To enable late module patching, livepatch modules need to be able to
> apply some of their relocations well after being loaded. In this
> scenario however, the livepatch module text and data is already RX-only,
> so special treatment is needed to make the late relocations possible. To
> do this, use the text-poking API for these late relocations.
>
> This patch is partially based off commit 88fc078a7a8f6 ("x86/module: Use
> text_poke() for late relocations").
>
> Signed-off-by: Dylan Hatch <dylanbhatch@google.com>
> Acked-by: Song Liu <song@kernel.org>
> ---
> arch/arm64/kernel/module.c | 110 ++++++++++++++++++++++---------------
> 1 file changed, 66 insertions(+), 44 deletions(-)
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 06bb680bfe975..93e6d63074afe 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -23,6 +23,7 @@
> #include <asm/insn.h>
> #include <asm/scs.h>
> #include <asm/sections.h>
> +#include <asm/text-patching.h>
>
> enum aarch64_reloc_op {
> RELOC_OP_NONE,
> @@ -48,7 +49,26 @@ static u64 do_reloc(enum aarch64_reloc_op reloc_op, __le32 *place, u64 val)
> return 0;
> }
>
> -static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
> +static void write_data(void *place, s64 *sval, size_t len, struct module *me)
> +{
> + if (me->state == MODULE_STATE_UNFORMED)
> + memcpy(place, sval, len);
> + else
> + aarch64_insn_copy(place, sval, len);
> +}
> +
> +static void write_insn(__le32 *place, u32 insn, struct module *me)
> +{
> + __le32 le = cpu_to_le32(insn);
> +
> + if (me->state == MODULE_STATE_UNFORMED)
> + *place = le;
> + else
> + aarch64_insn_copy(place, &le, sizeof(le));
> +}
Maybe combine these into a single macro such as below?
#define WRITE_PLACE(place, val, mod) do { \
if (mod->state == MODULE_STATE_UNFORMED) \
*(place) = val; \
else \
aarch64_insn_copy(place, &(val), sizeof(*place)); \
while (0)
> +static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len,
> + struct module *me)
> {
> s64 sval = do_reloc(op, place, val);
>
> @@ -66,7 +86,7 @@ static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
>
> switch (len) {
> case 16:
> - *(s16 *)place = sval;
> + write_data(place, &sval, sizeof(s16), me);
Then this would be:
WRITE_PLACE((s16 *)place, sval, me);
> @@ -145,7 +166,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);
> - *place = cpu_to_le32(insn);
> + write_insn(place, insn, me);
and this would be:
WRITE_PLACE(place, cpu_to_le32(insn), me);
Will
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5] arm64/module: Use text-poke API for late relocations.
2025-05-30 14:13 ` Will Deacon
@ 2025-05-31 0:11 ` Dylan Hatch
2025-06-03 15:13 ` Will Deacon
0 siblings, 1 reply; 5+ messages in thread
From: Dylan Hatch @ 2025-05-31 0:11 UTC (permalink / raw)
To: Will Deacon
Cc: Catalin Marinas, Ard Biesheuvel, Sami Tolvanen,
Geert Uytterhoeven, Song Liu, linux-arm-kernel, linux-kernel,
Roman Gushchin, Toshiyuki Sato
Hi Will,
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.
Thanks,
Dylan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5] arm64/module: Use text-poke API for late relocations.
2025-05-31 0:11 ` Dylan Hatch
@ 2025-06-03 15:13 ` Will Deacon
2025-06-03 17:25 ` Dylan Hatch
0 siblings, 1 reply; 5+ messages in thread
From: Will Deacon @ 2025-06-03 15:13 UTC (permalink / raw)
To: Dylan Hatch
Cc: Catalin Marinas, Ard Biesheuvel, Sami Tolvanen,
Geert Uytterhoeven, Song Liu, linux-arm-kernel, linux-kernel,
Roman Gushchin, Toshiyuki Sato
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;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5] arm64/module: Use text-poke API for late relocations.
2025-06-03 15:13 ` Will Deacon
@ 2025-06-03 17:25 ` Dylan Hatch
0 siblings, 0 replies; 5+ messages in thread
From: Dylan Hatch @ 2025-06-03 17:25 UTC (permalink / raw)
To: Will Deacon
Cc: Catalin Marinas, Ard Biesheuvel, Sami Tolvanen,
Geert Uytterhoeven, Song Liu, linux-arm-kernel, linux-kernel,
Roman Gushchin, Toshiyuki Sato
On Tue, Jun 3, 2025 at 8:13 AM Will Deacon <will@kernel.org> wrote:
>
> 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;
> }
>
>
This looks good to me. I can send a v7 with these changes.
Thanks,
Dylan
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-03 17:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-30 0:00 [PATCH v5] arm64/module: Use text-poke API for late relocations Dylan Hatch
2025-05-30 14:13 ` Will Deacon
2025-05-31 0:11 ` Dylan Hatch
2025-06-03 15:13 ` Will Deacon
2025-06-03 17:25 ` Dylan Hatch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).