[PATCH 1/6] powerpc/code-patching: Implement generic text patching function

[Christophe Leroy]

Le 19/09/2022 à 08:49, Benjamin Gray a écrit :
> On Mon, 2022-09-19 at 06:04 +0000, Christophe Leroy wrote:
>> With CONFIG_STRICT_KERNEL_RWX, this patches causes a 15% time
>> increase
>> for activation/deactivation of ftrace.
> 
> It's possible that new alignment check is the cause. I'll see
> 
> 
>> Without CONFIG_STRICT_KERNEL_RWX, it doesn't build.
> 
> Yup, fixed for v2
> 
>>> +static int __patch_text(void *dest, const void *src, size_t size,
>>> bool is_exec, void *exec_addr)
>>
>> Is 'text' a good name ? For me text mean executable code. Should it
>> be
>> __patch_memory() ?
> 
> Well patching regular memory is just a normal store. Text to me implies
> its non-writeable. Though __patch_memory would be fine.
> 
>> Why pass src as a void * ? This forces data to go via the stack.
>> Can't
>> you pass it as a 'long' ?
> 
> Probably, I wasn't aware that it would make a difference. I prefer
> pointers in general for their semantic meaning, but will change if it
> affects param passing.
> 
>>> +       if (virt_to_pfn(dest) != virt_to_pfn(dest + size - 1))
>>> +               return -EFAULT;
>>
>> Why do you need that new check ?
> 
> If the patch crosses a page boundary then letting it happen is
> unpredictable. Though perhaps this requirement can just be put as a
> comment, or require that patches be aligned to the patch size.

Why would it be unpredictable ? Only one page is mapped. If it crosses 
the boundary, __put_kernel_nofault() will fail in a controled manner.
I see no point in doing the check before every write.

Requiring an alignment to the patch size may be problematic when 
patching prefixed instructions (8 bytes). Allthough they are garantied 
to lie in a single cache line hence a single page, they are not 
garantied to be aligned to more than 4 bytes.

And while you are thinking about alignment, don't forget that dcbst and 
icbi apply on a give cacheline. If your memory crosses a cacheline you 
may have a problem.

> 
>>> +               case 8:
>>> +                       __put_kernel_nofault(dest, src, u64,
>>> failed);
>>> +                       break;
>>
>> Is case 8 needed for PPC32 ?
> 
> I don't have a particular need for it, but the underlying
> __put_kernel_nofault is capable of it so I included it.

Well, not including it will allow you to pass the source as a 'long' as 
mentionned above.

> 
>>> +       }
>>
>> Do you catch it when size if none of 1,2,4,8 ?
>>
> 
> Not yet. Perhaps I should wrap patch_text_data in a macro that checks
> the size with BUILD_BUG_ON? I'd rather not check at runtime.

Not necessarily a macro. WOuld be better as a static __always_inline in 
code_patching.h

> 
>>> +
>>> +       asm ("dcbst 0, %0; sync" :: "r" (dest));
>>
>> Maybe write it in C:
>>
>>          dcbst(dest);
>>          mb(); /* sync */
>>
>>> +
>>> +       if (is_exec)
>>> +               asm ("icbi 0,%0; sync; isync" :: "r" (exec_addr));
>>
>> Same, can be:
>>
>>          if (is_exec) {
>>                  icbi(exec_addr);
>>                  mb(); /* sync */
>>                  isync();
>>          }
>>
>> Or keep it flat:
>>
>>          if (!is_exec)
>>                  return 0;
>>
>>          icbi(exec_addr);
>>          mb(); /* sync */
>>          isync();
>>
>>          return 0;
> 
> Will try this.
> 
>>> +static int do_patch_text(void *dest, const void *src, size_t size,
>>> bool is_exec)
>>> +{
>>> +       int err;
>>> +       pte_t *pte;
>>> +       u32 *patch_addr;
>>> +
>>> +       pte = start_text_patch(dest, &patch_addr);
>>> +       err = __patch_text(patch_addr, src, size, is_exec, dest);
>>> +       finish_text_patch(pte);
>>
>> Why do you need to split this function in three parts ? I can't see
>> the
>> added value, all it does is reduce readability.
> 
> It made it more readable to me, so the __patch_text didn't get buried.
> It also made it easier to do the refactoring, and potentially add code
> patching variants that use the poke area but not __patch_text. I'll
> remove it for v2 though given this is the only use right now.
> 
>> Did you check the impact of calling __this_cpu_read() twice ?
> 
> I wasn't concerned about performance, but given I'll merge it back
> again it will only be read once in v2 again.
> 
>>> +void *patch_memory(void *dest, const void *src, size_t size)
>>
>> What is this function used for ?
>>
> 
> Build failure apparently :)
> 
> It's removed in v2.
>>