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