Re: [RFC 2/5] xen/livepatch: Allow tracers to schedule livepatch

[Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>]

Hi Ryo,

Thanks for adding this interface for tracers. I'd like to help with the
review, although I'm still getting familiar with this code and have used
LLM assistance. I may have missed part of the result-handling contract;
could you help me check the following failure path?

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1823,6 +1834,51 @@ static int schedule_work(struct payload *data, 
> uint32_t cmd, uint32_t timeout)
> +int livepatch_schedule_trace_on(struct payload *payload, struct 
> livepatch_func *func, unsigned long ip)
> +{
> +    void *old_addr;
> +    int rc;
> +
> +    spin_lock(&payload_lock);
> +    if ( is_work_scheduled(payload) )
> +    {
> +        rc = -EBUSY;
> +        goto out;
> +    }
> +
> +    old_addr = func->old_addr;
> +    func->old_addr = (void *)ip;
> +
> +    payload->rc = -EAGAIN;
> +    rc = schedule_work(payload, LIVEPATCH_TRACE_ON, 0);
> +    if ( rc )
> +        func->old_addr = old_addr;

Am I right that is_work_scheduled(payload) only detects work for this
particular payload, rather than any work using the shared worker?

For reference, the existing is_work_scheduled() in the same file ends
with this check, which is not added by this patch:

>     return livepatch_work.do_work && livepatch_work.data == data;

The existing schedule_work() instead has this global busy check:

>     /* Fail if an operation is already scheduled. */
>     if ( livepatch_work.do_work )
>         return -EBUSY;

Could a request therefore pass the first check while another payload
has work pending, set its own result to -EAGAIN, and then be rejected
by schedule_work()? Failure to acquire the CPU-map lock seems to offer
another path to the same outcome.

The caller does receive the immediate submission error, but wouldn't
payload->rc still suggest that an operation is pending, with no queued
worker for this payload to complete it?

The off helper appears to have the same result-handling question:

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1823,6 +1834,51 @@ static int schedule_work(struct payload *data, 
> uint32_t cmd, uint32_t timeout)
> +int livepatch_schedule_trace_off(struct payload *payload)
> +{
> [...]
> +    payload->rc = -EAGAIN;
> +    rc = schedule_work(payload, LIVEPATCH_TRACE_OFF, 0);
> +
> +    spin_unlock(&payload_lock);
> +
> +    return rc;
> +}

Would publishing the submission error in payload->rc keep these rejected
requests distinct from work that is still pending?

To illustrate, I think this could be addressed with a small change like:

--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1861,7 +1861,10 @@ int livepatch_schedule_trace_on(struct payload *payload, 
struct livepatch_func *func, unsigned long ip)
     payload->rc = -EAGAIN;
     rc = schedule_work(payload, LIVEPATCH_TRACE_ON, 0);
     if ( rc )
+    {
         func->old_addr = old_addr;
+        payload->rc = rc;
+    }
 
 out:
     spin_unlock(&payload_lock);
@@ -1882,6 +1885,8 @@ int livepatch_schedule_trace_off(struct payload *payload)
 
     payload->rc = -EAGAIN;
     rc = schedule_work(payload, LIVEPATCH_TRACE_OFF, 0);
+    if ( rc )
+        payload->rc = rc;
 
     spin_unlock(&payload_lock);
 

Would this preserve the pending assignment before successful queuing,
while leaving a terminal error when no operation was accepted? This
only handles immediate submission failure; it would not reconcile the
filter or target state with a later worker failure or make retargeting
an applied payload safe.

Thanks for taking a look; is there an intended status convention that
would make this harmless?

cu, Bernhard

-- 
Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>