lkml.org 
[lkml]   [2014]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 13/16] ARM: Add an emulate flag to the kprobes/uprobes instruction decode functions
On 01/16/14 04:18, Jon Medhurst (Tixy) wrote:
> On Wed, 2014-01-15 at 14:31 -0500, David Long wrote:
>> On 12/20/13 09:58, Jon Medhurst (Tixy) wrote:
>>> On Sun, 2013-12-15 at 23:08 -0500, David Long wrote:
> [...]
>>>> {
>>>> #ifdef CONFIG_THUMB2_KERNEL
>>>> if (thumb) {
>>>> @@ -253,7 +253,7 @@ set_emulated_insn(probes_opcode_t insn, struct arch_specific_insn *asi,
>>>> * non-zero value, the corresponding nibble in pinsn is validated and modified
>>>> * according to the type.
>>>> */
>>>> -static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs)
>>>> +static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs, bool modify)
>>>> {
>>>> probes_opcode_t insn = *pinsn;
>>>> probes_opcode_t mask = 0xf; /* Start at least significant nibble */
>>>> @@ -317,9 +317,16 @@ static bool __kprobes decode_regs(probes_opcode_t *pinsn, u32 regs)
>>>> /* Replace value of nibble with new register number... */
>>>> insn &= ~mask;
>>>> insn |= new_bits & mask;
>>>> + if (modify) {
>>>> + /* Replace value of nibble with new register number */
>>>> + insn &= ~mask;
>>>> + insn |= new_bits & mask;
>>>> + }
>>>
>>> Huh? As is, the above addition doesn't do anything because insn has
>>> already been modified. I guess you played with the idea that you needed
>>> to avoid changing insn (you don't) and then didn't undo the experiment
>>> quite right. :-)
>>>
>>
>> The conditional modification of the instruction was part of Rabin's
>> original work for uprobes, but I messed up the merge from an earlier
>> working version of my patches. My intention was/is to delete the old
>> unconditional code. Sounds like maybe you disagree though. The intent
>> is to only modify the instruction in the kprobes case.
>
> 'insn' is the local variable containing the instruction value we're
> processing. It doesn't matter if we change that, we just need to avoid
> updating the instruction in memory, which the code in the next chunk
> already correctly checks for...
>
>>>> }
>>>>
>>>> - *pinsn = insn;
>>>> + if (modify)
>>>> + *pinsn = insn;
>>>> +
>>>> return true;
>>>>
>
> So only one of these 'if (modify)' checks is required for code
> correctness, and I suggest keeping the second one as it's more explicit
> and defensive.
>
>

OK, I see your point. I shall simplify the code as you have suggested.

-dl




\
 
 \ /
  Last update: 2014-01-16 20:01    [W:0.061 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site