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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxx>]

On Mon, Dec 11, 2023 at 10:42 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Wed, Dec 06, 2023 at 12:11:39PM +0000, Ross Lagerwall wrote:
> > 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.
>
> Keep in mind livepatch-build-tools builds it's base version of Xen and
> then a patched version, and that's how the diff is performed.  For the
> hostname/domain/username changes to appear on the livepatch payload
> such change would need to happen in the muddle of the execution of
> livepatch-build.
>
> This change doesn't prevent building the original Xen on a different
> host than the one building the livepatch, and the
> hostname/domain/username changes won't be part of the livepatch
> payload.

Oh, of course. That makes sense.

There is one other issue that may cause version.o to change
unnecessarily.

XEN_CHANGESET may change when the source tree is a Git repo.
In some cases, it will change from <commit> to <commit>-dirty
when the patch is applied. I guess we could set .scmversion
explicitly to avoid this?

>
> > >
> > > > 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.
>
> Will someone send a patch for this, or should I do it?
>

I will send a patch for this.

Thanks,
Ross