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:01:08AM +0200, Jan Beulich wrote:
> On 25.09.2024 10:42, 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>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Albeit ...
> 
> > @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
> >                                   PAGE_HYPERVISOR_RWX);
> >          flush_local(FLUSH_TLB_GLOBAL);
> >  
> > -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
> > -                            alt_done);
> > +        rc = _apply_alternatives(__alt_instructions, 
> > __alt_instructions_end,
> > +                                 alt_done);
> > +        if ( rc )
> > +            panic("Unable to apply alternatives: %d\n", rc);
> 
> ... I see little value in logging rc here - the other log message will
> provide better detail anyway.

Current log messages do indeed provide better detail, but maybe we end
up adding new return error paths to _apply_alternatives() in the
future.  I see no harm in printing the error code if there's one.

> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -896,7 +896,15 @@ static int prepare_payload(struct payload *payload,
> >                  return -EINVAL;
> >              }
> >          }
> > -        apply_alternatives(start, end);
> > +
> > +        rc = apply_alternatives(start, end);
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: 
> > %d\n",
> > +                   elf->name, rc);
> > +            return rc;
> > +        }
> 
> Whereas here it may indeed make sense to keep things as you have them, as the
> error code is propagated onwards, and matching it with other error messages
> coming from elsewhere may help in understanding the situation.
> 
> As to possible applying: It looks as if this was independent of the earlier 4
> patches?

Yes, I think patches 5 and 6 can be applied ahead of the preceding
livepatch fixes.

Thanks, Roger.