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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 23, 2024 at 09:45:15AM +0000, Ross Lagerwall wrote:
> 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()?

I've moved it to livepatch_upload(), as I think it's more natural to
be done together with the addition to the list of livepatches.  Thanks
for spotting it, I guess I got confused between free_payload_data()
free_payload().

Roger.