Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

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

On Wed, Sep 25, 2024 at 11:53:30AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > alternatives is used both at boot time, and when loading livepatch payloads.
> > While for the former it makes sense to panic, it's not useful for the 
> > later, as
> > for livepatches it's possible to fail to load the livepatch if alternatives
> > cannot be resolved and continue operating normally.
> >
> > Relax the BUGs in _apply_alternatives() to instead return an error code.  
> > The
> > caller will figure out whether the failures are fatal and panic.
> >
> > Print an error message to provide some user-readable information about what
> > went wrong.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Much nicer.  A few more diagnostic comments.
> 
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > index 7824053c9d33..c8848ba6006e 100644
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -198,9 +198,29 @@ static void init_or_livepatch 
> > _apply_alternatives(struct alt_instr *start,
> >          uint8_t buf[MAX_PATCH_LEN];
> >          unsigned int total_len = a->orig_len + a->pad_len;
> >  
> > -        BUG_ON(a->repl_len > total_len);
> > -        BUG_ON(total_len > sizeof(buf));
> > -        BUG_ON(a->cpuid >= NCAPINTS * 32);
> > +        if ( a->repl_len > total_len )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "alt replacement size (%#x) bigger than destination 
> > (%#x)\n",
> 
> These all say "some alternative went wrong", without identifying which. 
> For x86_decode_lite(), debugging was far easier when using:
> 
> "Alternative for %ps ...", ALT_ORIG_PTR(a)
> 
> If we get the order of loading correct, then %ps will even work for a
> livepatch, but that's secondary - even the raw number is slightly useful
> given the livepatch load address.

I don't think this will work as-is for livepatches.  The call to
register the virtual region is currently done in livepatch_upload(),
after load_payload_data() has completed.

We could see about registering the virtual region earlier (no
volunteering to do that work right now).

> I could be talked down to just "Alt for %ps" as you've got it here.  I
> think it's clear enough in context.  So, I'd recommend:
> 
> "Alt for %ps, replacement size %#x larger than origin %#x\n".
> 
> Here, I think origin is better than destination, when discussing
> alternatives.

Sure.

> I can adjust on commit.  Everything else is fine.

If you are comfortable with doing the adjustments on commit, please go
ahead.

Thanks, Roger.