linux-kernel - Re: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20181001132852eucas1p25ab2ddf39296e2c78b188234d168f814~ZfyJK4W4N0481804818eucas1p2j@eucas1p2.samsung.com>
Date:   Mon, 1 Oct 2018 15:28:51 +0200
From:   Maciej Slodczyk <m.slodczyk2@...tner.samsung.com>
To:     Julien Thierry <julien.thierry@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     linux@...linux.org.uk, oleg@...hat.com, catalin.marinas@....com,
        will.deacon@....com, peterz@...radead.org, mingo@...hat.com,
        acme@...nel.org, alexander.shishkin@...ux.intel.com,
        jolsa@...hat.com, namhyung@...nel.org, b.zolnierkie@...sung.com,
        m.szyprowski@...sung.com, k.lewandowsk@...sung.com
Subject: Re: [PATCH v2 5/7] arm64: make arm uprobes code reusable by arm64

Hi,

Thank you for the review.

> I think that it would be good to move the renaming changes out of this 
> patch.
> 

So, as I understand, you suggest separating renaming from moving and 
putting it in separate patches, right?

>>   })
>> +#define ARM_COMPAT_LR_OFFSET    0
> 
> Not sure this should be defined here. What's the meaning of compat for 
> arch/arm ?
> 

Sure, I agree that the name is not very fortunate. I'll change it to 
something like ARM_UPROBES_BRANCH_LR_OFFSET.

>> @@ -39,7 +39,7 @@ struct arch_uprobe {
>>       void (*posthandler)(struct arch_uprobe *auprobe,
>>                   struct arch_uprobe_task *autask,
>>                   struct pt_regs *regs);
>> -    struct arch_probes_insn asi;
>> +    struct arch_probes_insn api;
> 
> It would be easier to follow thing by making this change in its own 
> patch. (Probably before you move arm32 code to lib/probes)
> 

Yup.


>> +enum probes_insn {
>> +    INSN_REJECTED,
>> +    INSN_GOOD_NO_SLOT,
>> +    INSN_GOOD,
>> +};
> 
> Why have two definitions of this enum rather than a common one in 
> lib/probes?
> 

Will fix in v3.

>> -typedef void (probes_handler_t) (u32 opcode,
>> -               struct arch_probe_insn *api,
>> +typedef void (probes_insn_handler_t) (u32 opcode,
>> +               struct arch_probes_insn *api,
> 
> In the previous patch you were already aligning this handler the ARM32's 
> equivalent. Why not fix the name (for the handler and struct 
> arch_probes_insn) in the previous patch?
> 

OK.

>> +
>> +#define link_register(regs)            ((regs)->compat_lr)
>> +
>> +static inline void link_register_set(struct pt_regs *regs,
>> +                       unsigned long val)
>> +{
>> +    link_register(regs) = val;
>> +}
> 
> pstate.h isn't really related to compat mode and whichever compat 
> definition it contains the relations are made explicit through their names.
> 
> I don't think a macro "link_register" defined in arch/arm64 and visible 
> to any file including ptrace.h (which is a lot) should return 
> "compat_lr" instead of the actual link register.
> 
> I'd say have the link_register macro check whether "regs" refers to a 
> compat mode context or not and provide the adequate link register.
> 
> Otherwise maybe you can get away with naming the macro 
> "arm_link_register" and the macro "arm_link_register_set". But I would 
> prefer the previous approach.
> 

OK.

>> +#ifdef CONFIG_ARM64
>> +#include <../../../arm/include/asm/opcodes.h>
> 
> Hmmm not sure this is something that is accepted.
> 

OK, I'll fix it.

>> +/*
>> + * based on arm kprobes implementation
>> + */
>> +static void __kprobes simulate_ldm1stm1(probes_opcode_t insn,
>> +        struct arch_probes_insn *asi,
> 
> The whole asi/api mix become a bit confusing IMO.
> Should we have api when the argument is of type "arch_probes_insn" and 
> asi when the type is "arch_specific_insn"?
> Should we have more coherent definitions of those structures between arm 
> and arm64 if we are going to share functions between them?
> 

OK, I'll try to figure something out that's less confusing.

> 
> #ifdef CONFIG_ARM64
> 
>> +enum probes_insn
>> +uprobe_decode_ldmstm_aarch64(probes_opcode_t insn,
>> +         struct arch_probes_insn *asi,
>> +         const struct decode_header *d)
> 
> Should be static.
> 

OK.

Thanks again for the review. I'll rework the whole patchset to include 
your remarks.


Thank you,
Maciej