| Message ID | 20201113141823.58712-1-doebel@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] tools/xenstore: Log xenstored build ID on startup | expand |
On 13.11.2020 15:18, Bjoern Doebel wrote: > --- a/tools/xenstore/Makefile > +++ b/tools/xenstore/Makefile > @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS) > xenstored: LDFLAGS += $(SYSTEMD_LIBS) > endif > > +# xenstored: enforce creation of a buildID section and use a linker > +# script to add additional symbols around that section > +xenstored: LDFLAGS += -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld Since in the hypervisor build we're careful to not use --build-id when the linker doesn't support it, I suppose the same care needs applying here. See the setting of build_id_linker in ./Config.mk. Jan
On 13/11/2020 14:18, Bjoern Doebel wrote: > Right now we do not have a mechanism to determine the version of the > currently running xenstored at runtime. As xenstored runs throughout the > lifetime of a Xen host, this may lead to problems when newer user space > builds are staged. Then, the running xenstored will no longer match the > version of the installed xenstored. > > To allow users to always identify the running version of xenstored, add > a linker-generated unique build ID to every xenstored build. Add > functionality to log this build ID into a file upon service startup. > > Signed-off-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Martin Mazein <amazein@amazon.de> > Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> I understand the problem you're trying to solve, but why is this anything more than just enabling build-id's by default across tools/ ? There are already standard ways of interacting with the build id of running executables on the system. I'd strongly discourage doing anything custom in xenstored specifically. ~Andrew
On 13.11.20 15:18, Bjoern Doebel wrote: > Right now we do not have a mechanism to determine the version of the > currently running xenstored at runtime. As xenstored runs throughout the > lifetime of a Xen host, this may lead to problems when newer user space > builds are staged. Then, the running xenstored will no longer match the > version of the installed xenstored. > > To allow users to always identify the running version of xenstored, add > a linker-generated unique build ID to every xenstored build. Add > functionality to log this build ID into a file upon service startup. > > Signed-off-by: Bjoern Doebel <doebel@amazon.de> > Reviewed-by: Martin Mazein <amazein@amazon.de> > Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> No support for oxenstored or xenstore-stubdom? > --- > tools/hotplug/Linux/launch-xenstore.in | 2 +- > tools/xenstore/Makefile | 4 +++ > tools/xenstore/buildid_symbols.ld | 10 +++++++ > tools/xenstore/xenstored_core.c | 8 ++++++ > tools/xenstore/xenstored_core.h | 3 ++ > tools/xenstore/xenstored_minios.c | 4 +++ > tools/xenstore/xenstored_posix.c | 52 ++++++++++++++++++++++++++++++++++ > 7 files changed, 82 insertions(+), 1 deletion(-) > create mode 100644 tools/xenstore/buildid_symbols.ld > > diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in > index 991dec8d25..a6f2254030 100644 > --- a/tools/hotplug/Linux/launch-xenstore.in > +++ b/tools/hotplug/Linux/launch-xenstore.in > @@ -62,7 +62,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF > } > > echo -n Starting $XENSTORED... > - $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS > + $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid --buildid-file @XEN_RUN_DIR@/xenstored.buildid $XENSTORED_ARGS > > systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 > > diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile > index 9a0f0d012d..c63350980b 100644 > --- a/tools/xenstore/Makefile > +++ b/tools/xenstore/Makefile > @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS) > xenstored: LDFLAGS += $(SYSTEMD_LIBS) > endif > > +# xenstored: enforce creation of a buildID section and use a linker > +# script to add additional symbols around that section > +xenstored: LDFLAGS += -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld > + > $(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab) > > xenstored: $(XENSTORED_OBJS) > diff --git a/tools/xenstore/buildid_symbols.ld b/tools/xenstore/buildid_symbols.ld > new file mode 100644 > index 0000000000..d74024c4e9 > --- /dev/null > +++ b/tools/xenstore/buildid_symbols.ld > @@ -0,0 +1,10 @@ > +SECTIONS > +{ > + __buildid_note_section = . ; > + .note.gnu.build-id : > + { > + *(.note.gnu.build-id) > + } > + __buildid_end = . ; > +} > +INSERT AFTER .data > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index b4be374d3f..c6f107bdd9 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -1844,6 +1844,7 @@ static void usage(void) > > > static struct option options[] = { > + { "buildid-file", 1, NULL, 'B' }, > { "no-domain-init", 0, NULL, 'D' }, > { "entry-nb", 1, NULL, 'E' }, > { "pid-file", 1, NULL, 'F' }, > @@ -1875,12 +1876,16 @@ int main(int argc, char *argv[]) > bool outputpid = false; > bool no_domain_init = false; > const char *pidfile = NULL; > + const char *buildid_file = NULL; > int timeout; > > > while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", options, You are missing "B:" in the short options. > NULL)) != -1) { > switch (opt) { > + case 'B': > + buildid_file = optarg; > + break; > case 'D': > no_domain_init = true; > break; > @@ -1948,6 +1953,9 @@ int main(int argc, char *argv[]) > if (pidfile) > write_pidfile(pidfile); > > + if (buildid_file) > + write_buildid_file(buildid_file); > + > /* Talloc leak reports go to stderr, which is closed if we fork. */ > if (!dofork) > talloc_enable_leak_report_full(); You should update the usage printout, too. > diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h > index 1df6ad94ab..712280626c 100644 > --- a/tools/xenstore/xenstored_core.h > +++ b/tools/xenstore/xenstored_core.h > @@ -193,6 +193,9 @@ void xenbus_notify_running(void); > /* Write out the pidfile */ > void write_pidfile(const char *pidfile); > > +/* Write the buildid file */ > +void write_buildid_file(const char *buildidfile); > + > /* Fork but do not close terminal FDs */ > void daemonize(void); > /* Close stdin/stdout/stderr to complete daemonize */ > diff --git a/tools/xenstore/xenstored_minios.c b/tools/xenstore/xenstored_minios.c > index c94493e52a..ef1151aee4 100644 > --- a/tools/xenstore/xenstored_minios.c > +++ b/tools/xenstore/xenstored_minios.c > @@ -24,6 +24,10 @@ void write_pidfile(const char *pidfile) > { > } > > +void write_buildid_file(const char *buildid_file) > +{ > +} > + > void daemonize(void) > { > } > diff --git a/tools/xenstore/xenstored_posix.c b/tools/xenstore/xenstored_posix.c > index 1f9603fea2..ec017611d6 100644 > --- a/tools/xenstore/xenstored_posix.c > +++ b/tools/xenstore/xenstored_posix.c > @@ -20,6 +20,7 @@ > #include <sys/stat.h> > #include <unistd.h> > #include <fcntl.h> > +#include <stdint.h> > #include <stdlib.h> > #include <sys/mman.h> > > @@ -48,6 +49,57 @@ void write_pidfile(const char *pidfile) > close(fd); > } > > +/* > + * We don't have a working elf.h available here, so let's define our very own > + * data structs and accessor macros for ELF notes. > + * > + * https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html: > + * For 64–bit objects and 32–bit objects, each entry is an array of 4-byte > + * words in the format of the target processor. > + */ > +typedef struct > +{ > + uint32_t namesz; > + uint32_t descsz; > + uint32_t type; > +} elf_note_hdr; > + > +/* ELF Note accessors, copied from Xen's elf.h */ > +#define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3) > +#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_))) > +#define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + ELFNOTE_ALIGN((_n_)->namesz)) > +/* GNU LD: type == note (NT_GNU_BUILD_ID as in > + * https://sourceware.org/ml/binutils/2007-07/msg00012.html)*/ > +#define NT_GNU_BUILD_ID 3 > + > + > +void write_buildid_file(const char *buildid_file) > +{ > + unsigned int i = 0; > + FILE *fdesc; > + extern elf_note_hdr __buildid_note_section; > + unsigned int id_length = __buildid_note_section.descsz; > + char* desc = ELFNOTE_DESC(&__buildid_note_section); > + > + if (__buildid_note_section.type != NT_GNU_BUILD_ID) > + barf("Expected GNU_BUILDID note, but found type '%d'", > + __buildid_note_section.type); > + > + fdesc = fopen(buildid_file, "w+"); > + if (!fdesc) > + barf_perror("Error opening buildid file %s", buildid_file); > + > + /* We exit silently if daemon already running. */ > + if (lockf(fileno(fdesc), F_TLOCK, 0) == -1) > + exit(0); > + > + for (i = 0; i < id_length; ++i) > + fprintf(fdesc, "%02x", (unsigned char)desc[i]); > + fprintf(fdesc, "\n"); > + > + fclose(fdesc); > +} > + > /* Stevens. */ > void daemonize(void) > { > In general I don't like the approach using a file for this purpose. In case there is no generic solution possible, I'd rather have a XS_CONTROL sub command to get the current version from Xenstore. This will then at once cover xenstore-stubdom, too. Juergen
On 13.11.20 15:27, Jan Beulich wrote: > On 13.11.2020 15:18, Bjoern Doebel wrote: >> --- a/tools/xenstore/Makefile >> +++ b/tools/xenstore/Makefile >> @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS) >> xenstored: LDFLAGS += $(SYSTEMD_LIBS) >> endif >> >> +# xenstored: enforce creation of a buildID section and use a linker >> +# script to add additional symbols around that section >> +xenstored: LDFLAGS += -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld > Since in the hypervisor build we're careful to not use --build-id > when the linker doesn't support it, I suppose the same care needs > applying here. See the setting of build_id_linker in ./Config.mk. Ok, I will make this conditional on the setting of $(build_id_linker). > > Jan Bjoern Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 13.11.20 15:30, Andrew Cooper wrote: > On 13/11/2020 14:18, Bjoern Doebel wrote: >> Right now we do not have a mechanism to determine the version of the >> currently running xenstored at runtime. As xenstored runs throughout the >> lifetime of a Xen host, this may lead to problems when newer user space >> builds are staged. Then, the running xenstored will no longer match the >> version of the installed xenstored. >> >> To allow users to always identify the running version of xenstored, add >> a linker-generated unique build ID to every xenstored build. Add >> functionality to log this build ID into a file upon service startup. >> >> Signed-off-by: Bjoern Doebel <doebel@amazon.de> >> Reviewed-by: Martin Mazein <amazein@amazon.de> >> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> > I understand the problem you're trying to solve, but why is this > anything more than just enabling build-id's by default across tools/ ? > > There are already standard ways of interacting with the build id of > running executables on the system. I'd strongly discourage doing > anything custom in xenstored specifically. May I ask what tooling you would use to interact with a running process' buildid? > ~Andrew Bjoern Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 13.11.20 16:36, Jürgen Groß wrote: > On 13.11.20 15:18, Bjoern Doebel wrote: >> Right now we do not have a mechanism to determine the version of the >> currently running xenstored at runtime. As xenstored runs throughout the >> lifetime of a Xen host, this may lead to problems when newer user space >> builds are staged. Then, the running xenstored will no longer match the >> version of the installed xenstored. >> >> To allow users to always identify the running version of xenstored, add >> a linker-generated unique build ID to every xenstored build. Add >> functionality to log this build ID into a file upon service startup. >> >> Signed-off-by: Bjoern Doebel <doebel@amazon.de> >> Reviewed-by: Martin Mazein <amazein@amazon.de> >> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> > > No support for oxenstored or xenstore-stubdom? Your suggestion further down will apparently help for stubdom. I do not speak ocaml at all - how do we address this? > >> --- >> tools/hotplug/Linux/launch-xenstore.in | 2 +- >> tools/xenstore/Makefile | 4 +++ >> tools/xenstore/buildid_symbols.ld | 10 +++++++ >> tools/xenstore/xenstored_core.c | 8 ++++++ >> tools/xenstore/xenstored_core.h | 3 ++ >> tools/xenstore/xenstored_minios.c | 4 +++ >> tools/xenstore/xenstored_posix.c | 52 >> ++++++++++++++++++++++++++++++++++ >> 7 files changed, 82 insertions(+), 1 deletion(-) >> create mode 100644 tools/xenstore/buildid_symbols.ld >> >> diff --git a/tools/hotplug/Linux/launch-xenstore.in >> b/tools/hotplug/Linux/launch-xenstore.in >> index 991dec8d25..a6f2254030 100644 >> --- a/tools/hotplug/Linux/launch-xenstore.in >> +++ b/tools/hotplug/Linux/launch-xenstore.in >> @@ -62,7 +62,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons >> && . @CONFIG_DIR@/@CONFIG_LEAF >> } >> echo -n Starting $XENSTORED... >> - $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS >> + $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid --buildid-file >> @XEN_RUN_DIR@/xenstored.buildid $XENSTORED_ARGS >> systemd-notify --booted 2>/dev/null || timeout_xenstore >> $XENSTORED || exit 1 >> diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile >> index 9a0f0d012d..c63350980b 100644 >> --- a/tools/xenstore/Makefile >> +++ b/tools/xenstore/Makefile >> @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS) >> xenstored: LDFLAGS += $(SYSTEMD_LIBS) >> endif >> +# xenstored: enforce creation of a buildID section and use a linker >> +# script to add additional symbols around that section >> +xenstored: LDFLAGS += -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld >> + >> $(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab) >> xenstored: $(XENSTORED_OBJS) >> diff --git a/tools/xenstore/buildid_symbols.ld >> b/tools/xenstore/buildid_symbols.ld >> new file mode 100644 >> index 0000000000..d74024c4e9 >> --- /dev/null >> +++ b/tools/xenstore/buildid_symbols.ld >> @@ -0,0 +1,10 @@ >> +SECTIONS >> +{ >> + __buildid_note_section = . ; >> + .note.gnu.build-id : >> + { >> + *(.note.gnu.build-id) >> + } >> + __buildid_end = . ; >> +} >> +INSERT AFTER .data >> diff --git a/tools/xenstore/xenstored_core.c >> b/tools/xenstore/xenstored_core.c >> index b4be374d3f..c6f107bdd9 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -1844,6 +1844,7 @@ static void usage(void) >> static struct option options[] = { >> + { "buildid-file", 1, NULL, 'B' }, >> { "no-domain-init", 0, NULL, 'D' }, >> { "entry-nb", 1, NULL, 'E' }, >> { "pid-file", 1, NULL, 'F' }, >> @@ -1875,12 +1876,16 @@ int main(int argc, char *argv[]) >> bool outputpid = false; >> bool no_domain_init = false; >> const char *pidfile = NULL; >> + const char *buildid_file = NULL; >> int timeout; >> while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", >> options, > > You are missing "B:" in the short options. Ack. Fixing. > >> NULL)) != -1) { >> switch (opt) { >> + case 'B': >> + buildid_file = optarg; >> + break; >> case 'D': >> no_domain_init = true; >> break; >> @@ -1948,6 +1953,9 @@ int main(int argc, char *argv[]) >> if (pidfile) >> write_pidfile(pidfile); >> + if (buildid_file) >> + write_buildid_file(buildid_file); >> + >> /* Talloc leak reports go to stderr, which is closed if we >> fork. */ >> if (!dofork) >> talloc_enable_leak_report_full(); > > You should update the usage printout, too. Ok. > >> diff --git a/tools/xenstore/xenstored_core.h >> b/tools/xenstore/xenstored_core.h >> index 1df6ad94ab..712280626c 100644 >> --- a/tools/xenstore/xenstored_core.h >> +++ b/tools/xenstore/xenstored_core.h >> @@ -193,6 +193,9 @@ void xenbus_notify_running(void); >> /* Write out the pidfile */ >> void write_pidfile(const char *pidfile); >> +/* Write the buildid file */ >> +void write_buildid_file(const char *buildidfile); >> + >> /* Fork but do not close terminal FDs */ >> void daemonize(void); >> /* Close stdin/stdout/stderr to complete daemonize */ >> diff --git a/tools/xenstore/xenstored_minios.c >> b/tools/xenstore/xenstored_minios.c >> index c94493e52a..ef1151aee4 100644 >> --- a/tools/xenstore/xenstored_minios.c >> +++ b/tools/xenstore/xenstored_minios.c >> @@ -24,6 +24,10 @@ void write_pidfile(const char *pidfile) >> { >> } >> +void write_buildid_file(const char *buildid_file) >> +{ >> +} >> + >> void daemonize(void) >> { >> } >> diff --git a/tools/xenstore/xenstored_posix.c >> b/tools/xenstore/xenstored_posix.c >> index 1f9603fea2..ec017611d6 100644 >> --- a/tools/xenstore/xenstored_posix.c >> +++ b/tools/xenstore/xenstored_posix.c >> @@ -20,6 +20,7 @@ >> #include <sys/stat.h> >> #include <unistd.h> >> #include <fcntl.h> >> +#include <stdint.h> >> #include <stdlib.h> >> #include <sys/mman.h> >> @@ -48,6 +49,57 @@ void write_pidfile(const char *pidfile) >> close(fd); >> } >> +/* >> + * We don't have a working elf.h available here, so let's define our >> very own >> + * data structs and accessor macros for ELF notes. >> + * >> + * >> https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html: >> + * For 64–bit objects and 32–bit objects, each entry is an array of >> 4-byte >> + * words in the format of the target processor. >> + */ >> +typedef struct >> +{ >> + uint32_t namesz; >> + uint32_t descsz; >> + uint32_t type; >> +} elf_note_hdr; >> + >> +/* ELF Note accessors, copied from Xen's elf.h */ >> +#define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3) >> +#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_))) >> +#define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + >> ELFNOTE_ALIGN((_n_)->namesz)) >> +/* GNU LD: type == note (NT_GNU_BUILD_ID as in >> + * https://sourceware.org/ml/binutils/2007-07/msg00012.html)*/ >> +#define NT_GNU_BUILD_ID 3 >> + >> + >> +void write_buildid_file(const char *buildid_file) >> +{ >> + unsigned int i = 0; >> + FILE *fdesc; >> + extern elf_note_hdr __buildid_note_section; >> + unsigned int id_length = __buildid_note_section.descsz; >> + char* desc = ELFNOTE_DESC(&__buildid_note_section); >> + >> + if (__buildid_note_section.type != NT_GNU_BUILD_ID) >> + barf("Expected GNU_BUILDID note, but found type '%d'", >> + __buildid_note_section.type); >> + >> + fdesc = fopen(buildid_file, "w+"); >> + if (!fdesc) >> + barf_perror("Error opening buildid file %s", buildid_file); >> + >> + /* We exit silently if daemon already running. */ >> + if (lockf(fileno(fdesc), F_TLOCK, 0) == -1) >> + exit(0); >> + >> + for (i = 0; i < id_length; ++i) >> + fprintf(fdesc, "%02x", (unsigned char)desc[i]); >> + fprintf(fdesc, "\n"); >> + >> + fclose(fdesc); >> +} >> + >> /* Stevens. */ >> void daemonize(void) >> { >> > > In general I don't like the approach using a file for this purpose. > In case there is no generic solution possible, I'd rather have a > XS_CONTROL sub command to get the current version from Xenstore. This > will then at once cover xenstore-stubdom, too. > I will have a look into that. > > Juergen Bjoern Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On 13/11/2020 16:55, Bjoern Doebel wrote: > > On 13.11.20 15:30, Andrew Cooper wrote: >> On 13/11/2020 14:18, Bjoern Doebel wrote: >>> Right now we do not have a mechanism to determine the version of the >>> currently running xenstored at runtime. As xenstored runs throughout >>> the >>> lifetime of a Xen host, this may lead to problems when newer user space >>> builds are staged. Then, the running xenstored will no longer match the >>> version of the installed xenstored. >>> >>> To allow users to always identify the running version of xenstored, add >>> a linker-generated unique build ID to every xenstored build. Add >>> functionality to log this build ID into a file upon service startup. >>> >>> Signed-off-by: Bjoern Doebel <doebel@amazon.de> >>> Reviewed-by: Martin Mazein <amazein@amazon.de> >>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> >> I understand the problem you're trying to solve, but why is this >> anything more than just enabling build-id's by default across tools/ ? >> >> There are already standard ways of interacting with the build id of >> running executables on the system. I'd strongly discourage doing >> anything custom in xenstored specifically. > May I ask what tooling you would use to interact with a running > process' buildid? Amongst other things, yes. Although as Juergen points out, we want something which works with stub domains as well, and "normal userspace tools" won't cut it there. I still think a first patch in this series should be to turn build-id's on by default if supported by the toolchain, generally. ~Andrew
On 13/11/2020 16:56, Bjoern Doebel wrote: > On 13.11.20 16:36, Jürgen Groß wrote: >> On 13.11.20 15:18, Bjoern Doebel wrote: >>> Right now we do not have a mechanism to determine the version of the >>> currently running xenstored at runtime. As xenstored runs throughout >>> the >>> lifetime of a Xen host, this may lead to problems when newer user space >>> builds are staged. Then, the running xenstored will no longer match the >>> version of the installed xenstored. >>> >>> To allow users to always identify the running version of xenstored, add >>> a linker-generated unique build ID to every xenstored build. Add >>> functionality to log this build ID into a file upon service startup. >>> >>> Signed-off-by: Bjoern Doebel <doebel@amazon.de> >>> Reviewed-by: Martin Mazein <amazein@amazon.de> >>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> >> >> No support for oxenstored or xenstore-stubdom? > > Your suggestion further down will apparently help for stubdom. I do > not speak ocaml at all - how do we address this? CC'ing Edwin and Christian who have done the bulk of the oxenstored recently. It sounds like it might not be possible right now, but would be possible with a future plan to switch the Ocaml build system over to dune (the new/preferred Ocaml upstream toolchain). If it does end up being an XS_CONTROL sub-op, we can implement it at a future point when we can usefully answer the question. ~Andrew
On Fri, 2020-11-13 at 17:13 +0000, Andrew Cooper wrote: > On 13/11/2020 16:56, Bjoern Doebel wrote: > > On 13.11.20 16:36, Jürgen Groß wrote: > > > On 13.11.20 15:18, Bjoern Doebel wrote: > > > > Right now we do not have a mechanism to determine the version > > > > of the > > > > currently running xenstored at runtime. As xenstored runs > > > > throughout > > > > the > > > > lifetime of a Xen host, this may lead to problems when newer > > > > user space > > > > builds are staged. Then, the running xenstored will no longer > > > > match the > > > > version of the installed xenstored. > > > > > > > > To allow users to always identify the running version of > > > > xenstored, add > > > > a linker-generated unique build ID to every xenstored build. > > > > Add > > > > functionality to log this build ID into a file upon service > > > > startup. > > > > > > > > Signed-off-by: Bjoern Doebel <doebel@amazon.de> > > > > Reviewed-by: Martin Mazein <amazein@amazon.de> > > > > Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> > > > > > > No support for oxenstored or xenstore-stubdom? > > > > Your suggestion further down will apparently help for stubdom. I do > > not speak ocaml at all - how do we address this? > > CC'ing Edwin and Christian who have done the bulk of the oxenstored > recently. > > It sounds like it might not be possible right now, but would be > possible > with a future plan to switch the Ocaml build system over to dune (the > new/preferred Ocaml upstream toolchain). See here what is possible with Dune: https://dune.readthedocs.io/en/stable/dune-libs.html#build-info Would the output of 'git describe --always --dirty' (perhaps combined with a build date) serve as a useful build ID? > > If it does end up being an XS_CONTROL sub-op, we can implement it at > a > future point when we can usefully answer the question. Wouldn't using readelf on /proc/<pid>/exe give you the running buildid? readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep 'Build ID' Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep 'Build ID' Build ID: b44ff99b216db7526e3ee7841068d584cc9c2b95 Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae When you're inside a stubdom it is probably not so easy though. Best regards, --Edwin > > ~Andrew
On 13.11.20 18:23, Edwin Torok wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > On Fri, 2020-11-13 at 17:13 +0000, Andrew Cooper wrote: >> On 13/11/2020 16:56, Bjoern Doebel wrote: >>> On 13.11.20 16:36, Jürgen Groß wrote: >>>> On 13.11.20 15:18, Bjoern Doebel wrote: >>>>> Right now we do not have a mechanism to determine the version >>>>> of the >>>>> currently running xenstored at runtime. As xenstored runs >>>>> throughout >>>>> the >>>>> lifetime of a Xen host, this may lead to problems when newer >>>>> user space >>>>> builds are staged. Then, the running xenstored will no longer >>>>> match the >>>>> version of the installed xenstored. >>>>> >>>>> To allow users to always identify the running version of >>>>> xenstored, add >>>>> a linker-generated unique build ID to every xenstored build. >>>>> Add >>>>> functionality to log this build ID into a file upon service >>>>> startup. >>>>> >>>>> Signed-off-by: Bjoern Doebel <doebel@amazon.de> >>>>> Reviewed-by: Martin Mazein <amazein@amazon.de> >>>>> Reviewed-by: Paul Durrant <pdurrant@amazon.co.uk> >>>> No support for oxenstored or xenstore-stubdom? >>> Your suggestion further down will apparently help for stubdom. I do >>> not speak ocaml at all - how do we address this? >> CC'ing Edwin and Christian who have done the bulk of the oxenstored >> recently. >> >> It sounds like it might not be possible right now, but would be >> possible >> with a future plan to switch the Ocaml build system over to dune (the >> new/preferred Ocaml upstream toolchain). > See here what is possible with Dune: > https://dune.readthedocs.io/en/stable/dune-libs.html#build-info > > Would the output of 'git describe --always --dirty' (perhaps combined > with a build date) serve as a useful build ID? The point of the build ID is to verify something like "binary-equivalence" of two builds. * a git hash is not sufficient because different git hashes may result in the same binary to be created (i.e., if there is no code change in the target binary in between those two builds) * a time stamp is counter-productive, because then you'd have to recreate this timestamp every time you want to re-create a build GNU ld's --build-id claims to perform a checksumming of the "normative parts of the output contents". Whatever that means. ;) > >> If it does end up being an XS_CONTROL sub-op, we can implement it at >> a >> future point when we can usefully answer the question. > Wouldn't using readelf on /proc/<pid>/exe give you the running buildid? > > readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep > 'Build ID' > Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae > Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae > > readelf -a /usr/sbin/oxenstored /proc/$(pidof oxenstored)/exe | grep > 'Build ID' > Build ID: b44ff99b216db7526e3ee7841068d584cc9c2b95 > Build ID: bdd5304c8984ed22570d51308ae8717d03fe60ae > > > When you're inside a stubdom it is probably not so easy though. Interesting. I had not considered that because after upgrading xenstored to a different version, the running xenstored's /proc/$PID/exe shows as # ls -l /proc/$(pgrep xenstored)/exe lrwxrwxrwx 1 root root 0 Nov 9 14:06 /proc/3528/exe -> /usr/sbin/xenstored (deleted) But you are right, one can still read that procfs file. Nice! Bjoern Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
How about keeping such an ID in xenstore itself in some kind of /meta hierarchy where xenstore could also keep stats? As long xenstore is running this information is easily accessible for outside tools. -- C
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in index 991dec8d25..a6f2254030 100644 --- a/tools/hotplug/Linux/launch-xenstore.in +++ b/tools/hotplug/Linux/launch-xenstore.in @@ -62,7 +62,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF } echo -n Starting $XENSTORED... - $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS + $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid --buildid-file @XEN_RUN_DIR@/xenstored.buildid $XENSTORED_ARGS systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1 diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile index 9a0f0d012d..c63350980b 100644 --- a/tools/xenstore/Makefile +++ b/tools/xenstore/Makefile @@ -66,6 +66,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS) xenstored: LDFLAGS += $(SYSTEMD_LIBS) endif +# xenstored: enforce creation of a buildID section and use a linker +# script to add additional symbols around that section +xenstored: LDFLAGS += -Wl,--build-id=sha1 -Wl,-T,buildid_symbols.ld + $(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab) xenstored: $(XENSTORED_OBJS) diff --git a/tools/xenstore/buildid_symbols.ld b/tools/xenstore/buildid_symbols.ld new file mode 100644 index 0000000000..d74024c4e9 --- /dev/null +++ b/tools/xenstore/buildid_symbols.ld @@ -0,0 +1,10 @@ +SECTIONS +{ + __buildid_note_section = . ; + .note.gnu.build-id : + { + *(.note.gnu.build-id) + } + __buildid_end = . ; +} +INSERT AFTER .data diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index b4be374d3f..c6f107bdd9 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1844,6 +1844,7 @@ static void usage(void) static struct option options[] = { + { "buildid-file", 1, NULL, 'B' }, { "no-domain-init", 0, NULL, 'D' }, { "entry-nb", 1, NULL, 'E' }, { "pid-file", 1, NULL, 'F' }, @@ -1875,12 +1876,16 @@ int main(int argc, char *argv[]) bool outputpid = false; bool no_domain_init = false; const char *pidfile = NULL; + const char *buildid_file = NULL; int timeout; while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", options, NULL)) != -1) { switch (opt) { + case 'B': + buildid_file = optarg; + break; case 'D': no_domain_init = true; break; @@ -1948,6 +1953,9 @@ int main(int argc, char *argv[]) if (pidfile) write_pidfile(pidfile); + if (buildid_file) + write_buildid_file(buildid_file); + /* Talloc leak reports go to stderr, which is closed if we fork. */ if (!dofork) talloc_enable_leak_report_full(); diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 1df6ad94ab..712280626c 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -193,6 +193,9 @@ void xenbus_notify_running(void); /* Write out the pidfile */ void write_pidfile(const char *pidfile); +/* Write the buildid file */ +void write_buildid_file(const char *buildidfile); + /* Fork but do not close terminal FDs */ void daemonize(void); /* Close stdin/stdout/stderr to complete daemonize */ diff --git a/tools/xenstore/xenstored_minios.c b/tools/xenstore/xenstored_minios.c index c94493e52a..ef1151aee4 100644 --- a/tools/xenstore/xenstored_minios.c +++ b/tools/xenstore/xenstored_minios.c @@ -24,6 +24,10 @@ void write_pidfile(const char *pidfile) { } +void write_buildid_file(const char *buildid_file) +{ +} + void daemonize(void) { } diff --git a/tools/xenstore/xenstored_posix.c b/tools/xenstore/xenstored_posix.c index 1f9603fea2..ec017611d6 100644 --- a/tools/xenstore/xenstored_posix.c +++ b/tools/xenstore/xenstored_posix.c @@ -20,6 +20,7 @@ #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> +#include <stdint.h> #include <stdlib.h> #include <sys/mman.h> @@ -48,6 +49,57 @@ void write_pidfile(const char *pidfile) close(fd); } +/* + * We don't have a working elf.h available here, so let's define our very own + * data structs and accessor macros for ELF notes. + * + * https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html: + * For 64–bit objects and 32–bit objects, each entry is an array of 4-byte + * words in the format of the target processor. + */ +typedef struct +{ + uint32_t namesz; + uint32_t descsz; + uint32_t type; +} elf_note_hdr; + +/* ELF Note accessors, copied from Xen's elf.h */ +#define ELFNOTE_ALIGN(_n_) (((_n_)+3)&~3) +#define ELFNOTE_NAME(_n_) ((char*)(_n_) + sizeof(*(_n_))) +#define ELFNOTE_DESC(_n_) (ELFNOTE_NAME(_n_) + ELFNOTE_ALIGN((_n_)->namesz)) +/* GNU LD: type == note (NT_GNU_BUILD_ID as in + * https://sourceware.org/ml/binutils/2007-07/msg00012.html)*/ +#define NT_GNU_BUILD_ID 3 + + +void write_buildid_file(const char *buildid_file) +{ + unsigned int i = 0; + FILE *fdesc; + extern elf_note_hdr __buildid_note_section; + unsigned int id_length = __buildid_note_section.descsz; + char* desc = ELFNOTE_DESC(&__buildid_note_section); + + if (__buildid_note_section.type != NT_GNU_BUILD_ID) + barf("Expected GNU_BUILDID note, but found type '%d'", + __buildid_note_section.type); + + fdesc = fopen(buildid_file, "w+"); + if (!fdesc) + barf_perror("Error opening buildid file %s", buildid_file); + + /* We exit silently if daemon already running. */ + if (lockf(fileno(fdesc), F_TLOCK, 0) == -1) + exit(0); + + for (i = 0; i < id_length; ++i) + fprintf(fdesc, "%02x", (unsigned char)desc[i]); + fprintf(fdesc, "\n"); + + fclose(fdesc); +} + /* Stevens. */ void daemonize(void) {