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

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

Hi Ryo,

Thanks for making the tracing controls available through HypFS. I'd like
to help review this part too, although I'm still getting familiar with
it and have used LLM assistance. I may have overlooked an existing
synchronization rule; could you help me check these cases?

> --- /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?
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?

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.

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

> --- /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?
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?

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

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

> --- /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? 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.

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

What is the intended contract for changing current_tracer while
tracing_on is still 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? 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? What should tracing_on report if the new
initialization fails and nop remains selected?

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

> --- /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 = .;

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?

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()?
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. 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?

cu, Bernhard

-- 
Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>