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

Le 19/09/2022 à 08:49, Benjamin Gray a écrit :

On Mon, 2022-09-19 at 06:04 +0000, Christophe Leroy wrote:

quoted

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

quoted

Without CONFIG_STRICT_KERNEL_RWX, it doesn't build.

Yup, fixed for v2

quoted

quoted

+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.

quoted

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.

quoted

quoted

+       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.

quoted

quoted

+               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.

quoted

quoted

+       }

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

quoted

quoted

+
+       asm ("dcbst 0, %0; sync" :: "r" (dest));

Maybe write it in C:

         dcbst(dest);
         mb(); /* sync */

quoted

+
+       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.

quoted

quoted

+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.

quoted

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.

quoted

quoted

+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.

quoted