Re: [PATCH 1/5] xen/livepatch: register livepatch regions when loaded

[Ross Lagerwall <ross.lagerwall@xxxxxxxxx>]

On Thu, Nov 30, 2023 at 2:30 PM Roger Pau Monne <roger.pau@xxxxxxxxxx> wrote:
>
> Currently livepatch regions are registered as virtual regions only after the
> livepatch has been applied.
>
> This can lead to issues when using the pre-apply or post-revert hooks, as at
> the point the livepatch is not in the virtual regions list.  If a livepatch
> pre-apply hook contains a WARN() it would trigger an hypervisor crash, as the
> code to handle the bug frame won't be able to find the instruction pointer 
> that
> triggered the #UD in any of the registered virtual regions, and hence crash.
>
> Fix this by adding the livepatch payloads as virtual regions as soon as 
> loaded,
> and only remove them once the payload is unloaded.  This requires some changes
> to the virtual regions code, as the removal of the virtual regions is no 
> longer
> done in stop machine context, and hence an RCU barrier is added in order to
> make sure there are no users of the virtual region after it's been removed 
> from
> the list.
>
> Fixes: 8313c864fa95 ('livepatch: Implement pre-|post- apply|revert hooks')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/common/livepatch.c      |  5 +++--
>  xen/common/virtual_region.c | 40 +++++++++++--------------------------
>  2 files changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 1209fea2566c..3199432f11f5 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -942,6 +942,8 @@ static int prepare_payload(struct payload *payload,
>          }
>      }
>
> +    register_virtual_region(region);
> +
>      return 0;
>  }
>

The region is registered in prepare_payload() but if e.g. the build id
check in load_payload_data() fails, it won't unregister the region
since the failure path calls free_payload_data(), not free_payload().
Perhaps the call to register_virtual_region() could be done as the last
thing in load_payload_data()?

Ross