linux-kernel - Re: [PATCH v6 5/6] LoongArch: Support PC-relative relocations in modules

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

Message-ID: <4033aec9-8df5-53ae-59e1-9ec3ade5f6d7@xen0n.name>
Date:   Tue, 30 Aug 2022 09:42:27 +0800
From:   WANG Xuerui <kernel@...0n.name>
To:     Huacai Chen <chenhuacai@...nel.org>, Xi Ruoyao <xry111@...111.site>
Cc:     loongarch@...ts.linux.dev, LKML <linux-kernel@...r.kernel.org>,
        Youling Tang <tangyouling@...ngson.cn>,
        Jinyang He <hejinyang@...ngson.cn>
Subject: Re: [PATCH v6 5/6] LoongArch: Support PC-relative relocations in
 modules

On 2022/8/29 23:08, Huacai Chen wrote:
> [snip]
>> +static int apply_r_larch_pcala(struct module *mod, u32 *location, Elf_Addr v,
>> +                       s64 *rela_stack, size_t *rela_stack_top, unsigned int type)
>> +{
>> +       union loongarch_instruction *insn = (union loongarch_instruction *)location;
>> +       /* Use s32 for a sign-extension deliberately. */
>> +       s32 offset_hi20 = (void *)((v + 0x800) & ~0xfff) -
>> +               (void *)((Elf_Addr)location & ~0xfff);
>> +       Elf_Addr anchor = (((Elf_Addr)location) & ~0xfff) + offset_hi20;
>> +       ptrdiff_t offset_rem = (void *)v - (void *)anchor;
>> +
>> +       switch (type) {
>> +       case R_LARCH_PCALA_HI20:
>> +               v = offset_hi20 >> 12;
>> +               break;
>> +       case R_LARCH_PCALA64_LO20:
>> +               v = offset_rem >> 32;
>> +               break;
>> +       case R_LARCH_PCALA64_HI12:
>> +               v = offset_rem >> 52;
>> +               break;
>> +       default:
>> +               /* Do nothing. */
>> +       }
>> +
>> +       switch (type) {
>> +       case R_LARCH_PCALA_HI20:
>> +       case R_LARCH_PCALA64_LO20:
>> +               insn->reg1i20_format.immediate = v & 0xfffff;
>> +               break;
>> +       case R_LARCH_PCALA_LO12:
>> +       case R_LARCH_PCALA64_HI12:
>> +               insn->reg2i12_format.immediate = v & 0xfff;
>> +               break;
>> +       default:
>> +               pr_err("%s: Unsupport relocation type %u\n", mod->name, type);
>> +               return -EINVAL;
>> +       }
> Can we merge the two switch here?

IMO leaving as-is or even splitting into two functions would be 
acceptable, as the two switches are performing two different things -- 
namely "adjustFixupValue" (in LLVM-speak) and actually inserting the 
value into the insn word. But an argument for merging the two can be 
made too, because the v2.00 reloc types are purposely designed with 
unique use case for each, meaning there is actually no flexibility in 
between the fixup value's calculation and application. So I think this 
eventually comes down to coder's preference?

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/