Re: [PATCH 1/3] xen/livepatch: simplify and unify logic in prepare_payload()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Sep 23, 2024 at 01:01:57PM +0200, Jan Beulich wrote:
> On 20.09.2024 11:36, Roger Pau Monne wrote:
> > The following sections: .note.gnu.build-id, .livepatch.xen_depends and
> > .livepatch.depends are mandatory and ensured to be present by
> > check_special_sections() before prepare_payload() is called.
> > 
> > Simplify the logic in prepare_payload() by introducing a generic function to
> > parse the sections that contain a buildid.  Note the function assumes the
> > buildid related section to always be present.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/common/livepatch.c | 106 ++++++++++++++++++-----------------------
> >  1 file changed, 46 insertions(+), 60 deletions(-)
> > 
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index d93a556bcda2..cea47ffe4c84 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -647,15 +647,37 @@ static inline int livepatch_check_expectations(const struct payload *payload)
> >      nhooks = __sec->sec->sh_size / sizeof(*hook);                                         \
> >  } while (0)
> >  
> > +static int fetch_buildid(const struct livepatch_elf_sec *sec,
> > +                         struct livepatch_build_id *id)
> > +{
> > +    const Elf_Note *n = sec->load_addr;
> > +    int rc;
> > +
> > +    ASSERT(sec);
> > +
> > +    if ( sec->sec->sh_size <= sizeof(*n) )
> > +        return -EINVAL;
> 
> Oh, after my reply to Andrew's reply, now looking at the actual change -
> is it perhaps ASSERT(sec->sec) that was meant?

The original check before moving the code was against 'sec', not
'sec->sec'.  That's what I intending to retain with the assert.

I can do the `n = sec->load_addr` assign after the assert if that's
better analysis wise.

Thanks, Roger.