Re: [PATCH] livepatch-build-tools: allow livepatching version.c

[Ross Lagerwall <ross.lagerwall@xxxxxxxxx>]

On Tue, Dec 5, 2023 at 2:57 PM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> > On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > > Currently version.o is explicitly ignored as the file would change as a 
> > > result
> > > of the orignal and the patched build having possibly different dates and
> > > times.
> > >
> > > Fix such difference by exporting the date and time from the build script, 
> > > so
> > > that both builds share the same build time.  This allows checking for 
> > > changes
> > > in version.c, since the rest of fields need to be manually changed in 
> > > order to
> > > produce different output.
> > >
> > > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been 
> > > supported
> > > since before livepatch support was added to Xen, so it's safe to export 
> > > those
> > > variables unconditionally.
> > >
> > > Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > ---
> > >  livepatch-build | 4 ++++
> > >  livepatch-gcc   | 2 --
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/livepatch-build b/livepatch-build
> > > index e2ccce4f7fd7..f622683fc56c 100755
> > > --- a/livepatch-build
> > > +++ b/livepatch-build
> > > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> > >
> > >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> > >
> > > +    # Force same date and time to prevent unwanted changes in version.c
> > > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> >
> > Date is the one that goes wrong every time, but everything else in
> > compile.h can go wrong in a way that causes version.o to change.
>
> I've attempted to reflect that in "since the rest of fields need to be
> manually changed in order to produce different output".
>
> For those to change there must be some kind of environment change
> between the original and the patched version build, and hence I don't
> think that would be supported.

In general, yes. However, with this patch changes to the
hostname/domain/username would result in version.o being marked
as changed even though it is entirely fine to build the live patch
on a different build host from the original Xen.

>
> > Ideally, the pristine source for building livepatches would include a
> > generated compile.h, and livepatch would have a way to force no
> > regeneration of the header.  But I've got no idea how nice that would be
> > to arrange.
>
> Yes, no idea how fragile that would be either.  IMO the proposed
> approach is not that bad.
>
> > That way, you're using the same details as the Xen being patched, rather
> > than hoping that two identical different details will cancel out in the
> > binary diff.
>
> Another option is to set all the env variables to disable any
> build time probing.  However things like compiler or version changing
> between the original and the patched builds likely point out to issues
> elsewhere, unless it's intentional modification of the helpers.
>
> > > +
> > >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> > >      build_full
> > >
> > > diff --git a/livepatch-gcc b/livepatch-gcc
> > > index fcad80551aa0..e4cb6fb59029 100755
> > > --- a/livepatch-gcc
> > > +++ b/livepatch-gcc
> > > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> > >              obj=$2
> > >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> > >              case "$(basename $obj)" in
> > > -            version.o|\
> > >              debug.o|\
> > >              check.o|\
> >
> > Tangential question.  check.o is excluded because it's a toolchain test,
> > but any idea what debug.o is doing in this list?
> >
> > It can't possibly be the debug.c I've recently added to x86 (which we'll
> > want to be able to livepatch), so I guess it's got something to do the
> > ARM debug.S's, but I can't see anything in those that are worthy of
> > exemption either...
>
> Hm, that comes from the first commit that imported the wrapper to the
> repository, and at that point only x86 had livepatch support.
>
> I'm tempted to think this was inherited from the original xsplice
> tooling, and so debug.o needs to be removed from the list.
>

livepatch-build-tools is derived from the kpatch build tooling and
debug.o has never been present there so it was added here for a
reason. AFAICT the gdbsx code used to live in debug.o. I can't
recall why it was being marked as changed unnecessarily but since
that is no longer an issue and the code lives elsewhere, the debug.o
lines can be dropped.

Ross