Re: [RFC 3/5] xen/tracing: prepare a livepatch payload and its controling interface

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

On Sat, 24 Oct 2026 16:13:58 +0200, Bernhard Kaindl wrote:
>> --- /dev/null
>> +++ b/xen/common/hypfs_trace.c
>> @@ -0,0 +1,741 @@
>> +void trace_dispatch_callbacks(unsigned long ip, unsigned long parent_ip)
>> +{
>> +    livepatch_trace_func_t callback;
>> +
>> +    if ( !tracing_is_on() )
>> +        return;
>> +
>> +    if ( !trace_callback_registered )
>> +        return;
>> +
>> +    if ( !global_trace_func.old_addr )
>> +        return;
>> +
>> +    if ( (unsigned long)global_trace_func.old_addr != ip )
>> +        return;
>> +
>> +    callback = global_trace_func.new_addr;
>> +    callback((unsigned long)global_trace_func.old_addr, parent_ip);
>> +}
>
>What synchronizes this reader with callback removal on another CPU?

Testing !trace_callback_registered was the intended synchronization here...

>The corresponding part of trace_unregister_callback() is:
>
>> --- /dev/null
>> +++ b/xen/common/hypfs_trace.c
>> @@ -0,0 +1,741 @@
>> +int trace_unregister_callback(void)
>> +{
>> +    int rc = 0;
>> +
>> +    spin_lock(&trace_callback_lock);
>> [...]
>> +    trace_callback_registered = false;
>> +    global_trace_func.new_addr = NULL;
>> +
>> +out_unlock:
>> +    spin_unlock(&trace_callback_lock);
>
>Could CPU A pass the enabled and registered tests, then CPU B clear
>new_addr, before CPU A loads and calls it? Would that permit a NULL
>indirect call despite the writer's lock, since the reader does not
>take that lock?

... and yes, I think this can happen.

>I wondered whether the existing payload RCU region might cover this,
>but the new dispatch appears after its unlock:
>
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -232,6 +236,11 @@ xen_livepatch_trace_dispatcher(unsigned long ip, 
>> unsigned long parent_ip)
>>  out:
>>      rcu_read_unlock(&rcu_payload_lock);
>> +
>> +#ifdef CONFIG_LIVEPATCH_TRACER
>> +    /* Call tracer callbacks. */
>> +    trace_dispatch_callbacks(ip, parent_ip);
>> +#endif
>
>Would an ordered publication and reader-lifetime protocol help here,
>including draining callbacks before unregister/reset completes? Could
>it cover registration as well, where the registered flag is currently
>set before the callback pointer? Merely checking the pointer for NULL
>would not seem to protect callback state already being used elsewhere.

Thank you for your suggestion.

I'll extend the existing payload RCU read-side region to cover the
callback dispatch and remove the registered flag entirely.

Do you think something like the following would work?

---- BEGIN IDEA ----
--- a/xen/common/hypfs_trace.c
+++ b/xen/common/hypfs_trace.c
@@ -135,17 +135,16 @@ void trace_dispatch_callbacks(unsigned long ip, unsigned 
long parent_ip)
     if ( !tracing_is_on() )
         return;

-    if ( !trace_callback_registered )
-        return;
-
     if ( !global_trace_func.old_addr )
         return;

     if ( arch_livepatch_trace_entry((unsigned long)global_trace_func.old_addr) 
!= ip )
         return;

-    callback = global_trace_func.new_addr;
-    callback((unsigned long)global_trace_func.old_addr, parent_ip);
+    callback = rcu_dereference(global_trace_func.new_addr);
+
+    if ( callback )
+        callback((unsigned long)global_trace_func.old_addr, parent_ip);
 }

 int trace_register_callback(livepatch_trace_func_t callback)
@@ -154,14 +153,13 @@ int trace_register_callback(livepatch_trace_func_t 
callback)

     spin_lock(&trace_callback_lock);

-    if ( trace_callback_registered )
+    if ( global_trace_func.new_addr )
     {
         rc = -EEXIST;
         goto out_unlock;
     }

-    trace_callback_registered = true;
-    global_trace_func.new_addr = callback;
+    rcu_assign_pointer(global_trace_func.new_addr, callback);

  out_unlock:
     spin_unlock(&trace_callback_lock);
@@ -175,14 +173,16 @@ int trace_unregister_callback(void)

     spin_lock(&trace_callback_lock);

-    if ( !trace_callback_registered )
+    if ( !global_trace_func.new_addr )
     {
         rc = -ENOENT;
         goto out_unlock;
     }

-    trace_callback_registered = false;
-    global_trace_func.new_addr = NULL;
+    rcu_assign_pointer(global_trace_func.new_addr, NULL);
+
+    /* Drain every callbacks before unregistering this tracer */
+    rcu_barrier();

 out_unlock:
     spin_unlock(&trace_callback_lock);
---- END IDEA ----

>Separately, could a pending filter apply survive a successful clear?

Yes, it could.
Currently, it only checks the immediate result of schedule_work()
and the applied state of the payload.

>> --- /dev/null
>> +++ b/xen/common/hypfs_trace.c
>> @@ -0,0 +1,741 @@
>> +    /* Schedule removal of the current patch. */
>> +    if ( global_trace_fstate.applied == LIVEPATCH_FUNC_APPLIED )
>> [...]
>> +    /* Clear the filter if no target function was requested. */
>> +    if ( !ip )
>> +    {
>> +        /*
>> +         * Keep global_trace_func.old_addr since livepatching
>> +         * might be in flight. Only clear global_trace.filter_name
>> +         * which will effectively update /tracing/set_function_filter.
>> +         */
>> +        global_trace.filter_name[0] = '\0';
>> +        return 0;
>> +    }
>
>If I select a function and then clear the filter before the trace-on
>worker runs, wouldn't applied still be LIVEPATCH_FUNC_NOT_APPLIED?

Yes.

>Could the clear therefore return success without canceling or removing
>anything, followed by the queued worker installing the original target
>while set_function_filter reports an empty filter?

Yes.

>Would it make sense to track pending work explicitly and either return
>busy or arrange a safe cancellation/removal before accepting the clear?

Yes, I agree tracking the pending work. I'll make sure the pending work
gets handled appropriately for the next version.
And I believe it should take care the issue you addressed concerning
trace_set_filter() as well.

>There seems to be a related completion-status question at the end of
>trace_set_filter():

Yes.

>> --- /dev/null
>> +++ b/xen/common/hypfs_trace.c
>> @@ -0,0 +1,741 @@
>> +    rc = livepatch_schedule_trace_on(payload, global_trace.func, ip);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    strlcpy(global_trace.filter_name, name, 
>> sizeof(global_trace.filter_name));
>
>Am I missing a place where the controls consume payload->rc after the
>worker finishes?

No, you are right. There is currently no such place.

>The existing livepatch worker can fail during CPU
>rendezvous or application after submission has succeeded. Could the
>displayed name then describe a target that was never installed? The
>removal path clears the name before completion, which seems to allow
>the converse if removal fails.

Yes. The same issue stemming from overlooking the pending work.

>Could requested and installed state be kept separate until completion,
>with a way for the user to see the eventual result?

I agree that those two states should be kept separate.

I plan to prepare a new file, something like /set_function_status,
which user can track the state of the request, which is payload->rc.
And leave /set_function_filter for showing the installed function.

>What is the intended contract for changing current_tracer while
>tracing_on is still set?

I see that its inconsistent right now.
I think current_tracer should not be changed while tracing_on is set.

>> --- /dev/null
>> +++ b/xen/common/hypfs_trace.c
>> @@ -0,0 +1,741 @@
>> +    if ( prev->reset )
>> +    {
>> +        rc = prev->reset(&global_trace);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    global_trace.current_trace = &nop_tracer;
>> +
>> +    if ( new->init )
>> +    {
>> +        rc = new->init(&global_trace);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    global_trace.current_trace = new;
>
>Could a tracer change either be rejected while recording, or perform
>a stop-and-drain transition before reset and initialization?

I plan fix it by the former approach of "reject while recording".

>For a tracer using the optional start/stop hooks, wouldn't the present path
>leave recording reported as enabled without stopping the old tracer
>or starting the new one?

I missed this inconsistency as well.
I believe it should also be fixed by enforcing "reject while recording".

>What should tracing_on report if the new
>initialization fails and nop remains selected?

By enforcing "reject while recording", it should be reporting '0' as
tracing should already be disabled at this point.

>One more target-validation question: what excludes discarded init
>functions from filter selection after boot?

Thank you. This was missing as well.

>> --- /dev/null
>> +++ b/xen/common/hypfs_trace.c
>> @@ -0,0 +1,741 @@
>> +    if ( len )
>> +    {
>> +        ip = symbols_lookup_by_name(name);
>> +        if ( !ip || !is_patchable_function_entry(ip) )
>> +            return -EINVAL;
>> +    }
>
>The new linker table appears to retain the compiler-generated entries
>without filtering out init functions:
>
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -61,6 +61,11 @@ SECTIONS
>> +       . = ALIGN(8);
>> +       __start_patchable_function_entries = .;
>> +       KEEP(*(__patchable_function_entries))
>> +       __stop_patchable_function_entries = .;

Yes, they still retain the init functions.

>For reference, the existing symbol_valid() in xen/tools/symbols.c
>retains symbols in init text too, and symbols_lookup_by_name() in
>xen/common/symbols.c returns matching addresses without an active-text
>check. Its non-FAST_SYMBOL_LOOKUP branch contains:
>
>>         if ( !strcmp(name, symname) )
>>             return addr;
>
>Meanwhile free_init_memory() in xen/arch/arm/mmu/setup.c removes the
>init mapping and returns those pages to the heap when appropriate:
>
>>     rc = destroy_xen_mappings((unsigned long)__init_begin,
>>                               (unsigned long)__init_end);
>> [...]
>>     if ( !using_static_heap )
>>     {
>>         init_domheap_pages(pa, pa + len);
>
>Could an init function's name and compiler entry therefore still pass
>this selection check after its code has been discarded? Since the Arm
>patcher writes through an alias of the original image's physical pages,
>could that write into memory which has already been reused?

Thank you for elaborating.
And yes, currently init functions could pass the check and be selected.

>Would checking active text before accepting the target address rule out
>this case? To illustrate, I think this could be addressed with a small
>change like:
>
>--- a/xen/common/hypfs_trace.c
>+++ b/xen/common/hypfs_trace.c
>@@ -3,6 +3,7 @@
> #include <xen/init.h>
> #include <xen/hypfs.h>
> #include <xen/hypfs_trace.h>
>+#include <xen/kernel.h>
> #include <xen/livepatch_payload.h>
> #include <xen/symbols.h>
> #include <xen/string.h>
>@@ -266,7 +267,8 @@ static int trace_set_filter(const char *name)
>     if ( len )
>     {
>         ip = symbols_lookup_by_name(name);
>-        if ( !ip || !is_patchable_function_entry(ip) )
>+        if ( !ip || !is_active_kernel_text(ip) ||
>+             !is_patchable_function_entry(ip) )
>             return -EINVAL;
>     }
> 
>
>Would this reject discarded init text before either scheduling path,
>using the same text-region safeguard as address-based symbols_lookup()?

Yes, I think this works.
I'll make the change for the next version.

>This only checks whether the target is currently active text. It does
>not validate reserved instruction bytes, reject overlapping livepatches,
>or establish the lifetime of a dynamically removable target.

Understood.
For the other concerns, please take a look at my reply to your feedback
on the 1/5 patch. I believe the check I added for the proposed
arch_livepatch_verify_entry() should address those concerns.

>The pending
>operation and callback-lifetime questions above would still need their
>own solution.
>
>Thanks for looking through these questions. Is there a lifecycle,
>completion, or target-lifetime guarantee elsewhere that I have missed?

Thank you for your feedback. You didn't miss anything.
I'll make sure they get fixed for the next version.

Sincerely,
Ryo Takakura