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

[Ryo Takakura <takakura@xxxxxxxxxxxxx>]

On Sat, 24 Oct 2026 16:09:36 +0200 Bernhard Kaindl wrote:
>> --- 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?

Yes, it only checks for this particular payload.

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

Yes, payload->rc will keep indicating that the request is pending.

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

Yes, it has the same issue.

>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;

Thank you. This looks good to me.
I'll include your fix in the next version.

>it would not reconcile the
>filter or target state with a later worker failure or make retargeting
>an applied payload safe.

Thank you for pointing this out as well.
I see that the failure after scheduling needs to be handled separately.
I'll think how to handle those cases for the next version.

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

When implementing these two functions, I followed the pattern in
livepatch_action(). And docs/misc/livepatch.pandoc says that -EAGAIN
indicates "operation in progress". Leaving payload->rc at -EAGAIN after
failing schedule_work() appears to be inconsistent with that convention.

Would it make sense to fix the error handling in livepatch_action() as well?

Suggested fix:
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -2276,6 +2276,8 @@ static int livepatch_action(struct 
xen_sysctl_livepatch_action *action)

             data->rc = -EAGAIN;
             rc = schedule_work(data, action->cmd, action->timeout);
+            if ( rc )
+                data->rc = rc;
         }
         break;

Sincerely,
Ryo Takakura