Re: [Xen-devel] [PATCH v8.1 24/27] xsplice: Stacking build-id dependency checking.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:03 AM >>>
>+.PHONY: note.o
>+note.o:
>+      $(OBJCOPY) -O binary --only-section=.note.gnu.build-id 
>$(BASEDIR)/xen-syms $@.bin
>+      $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \
>+                 --rename-section=.data=.xsplice.depends -S $@.bin $@
>+      rm -f $@.bin

Same note as on the earlier patch regarding an interrupted build here ...

>+.PHONY: hello_world_note.o
>+hello_world_note.o:
>+      $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(XSPLICE) $@.bin
>+      $(OBJCOPY)  -I binary -O elf64-x86-64 -B i386:x86-64 \
>+                 --rename-section=.data=.xsplice.depends -S $@.bin $@
>+      rm -f $@.bin

... and here. Thinking about it a second time, wouldn't that then also require
entries in .gitignore?

>$(MAKE) -f $(BASEDIR)/Rules.mk xen_hello_world_func.o
        >$(MAKE) -f $(BASEDIR)/Rules.mk xen_hello_world.o
>-      $(LD) $(LDFLAGS) -r -o $(XSPLICE) xen_hello_world_func.o \
>-              xen_hello_world.o
>+      $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE) \
>+              xen_hello_world_func.o xen_hello_world.o note.o
>+      $(MAKE) -f $(BASEDIR)/Rules.mk xen_bye_world_func.o
>+      $(MAKE) -f $(BASEDIR)/Rules.mk xen_bye_world.o
>+      $(MAKE) -f $(BASEDIR)/Rules.mk hello_world_note.o
>+      $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE_BYE) \
>+              xen_bye_world_func.o xen_bye_world.o hello_world_note.o

Coming back to the remark on the much earlier patch: The latest here it should
become pretty clear that this explicit invocation of make is getting unwieldy.

>--- /dev/null
>+++ b/xen/arch/x86/test/xen_bye_world.c

Any comments previously given on its "hello" sibling would likely apply here 
too.

>--- a/xen/common/version.c
>+++ b/xen/common/version.c
>@@ -84,9 +84,38 @@ int xen_build_id(const void **p, unsigned int *len)
 >/* Defined in linker script. */
 >extern const Elf_Note __note_gnu_build_id_start[], __note_gnu_build_id_end[];
 >
>+int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
>+                       const void **p, unsigned int *len)
>+{
>+    /* Check if we really have a build-id. */
>+    if ( NT_GNU_BUILD_ID != n->type )
>+        return -ENODATA;
>+
>+    if ( n->namesz >= n_sz )
>+        return -EINVAL;
>+
>+    if ( n->descsz >= n_sz )
>+        return -EINVAL;
>+
>+    if ( n->namesz + n->descsz >= n_sz )
>+        return -EINVAL;

These three checks could be done by two ones, and for one more correctly:
First check that the addition doesn't overflow, and then do the latter check.
But then it looks like the addition should also include sizeof(*n) (and then
use > instead of >=)?

>+    /* Sanity check, name should be "GNU" for ld-generated build-id. */
>+    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
>+        return -ENODATA;

So if namesz is 3, this will (I think wrongly) pass as it seems.

>@@ -96,18 +125,9 @@ static int __init xen_build_init(void)
     >if ( &n[1] > __note_gnu_build_id_end )
         >return -ENODATA;;
 >
>-    /* Check if we really have a build-id. */
>-    if ( NT_GNU_BUILD_ID != n->type )
>-        return -ENODATA;
>-
>-    /* Sanity check, name should be "GNU" for ld-generated build-id. */
>-    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
>-        return -ENODATA;
>-
>-    build_id_len = n->descsz;
>-    build_id_p = ELFNOTE_DESC(n);
>+    sz = (size_t)__note_gnu_build_id_end - (size_t)&n[0];
 
Why &n[0] instead of just n?

>@@ -441,6 +452,7 @@ static int prepare_payload(struct payload *payload,
     >unsigned int i;
     >struct xsplice_patch_func_internal *f;
     >struct virtual_region *region;
>+    Elf_Note *n;
 
const
     
>@@ -1252,6 +1343,18 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
>*action)
     >case XSPLICE_ACTION_REVERT:
         >if ( data->state == XSPLICE_STATE_APPLIED )
         >{
>+            struct payload *p = list_last_entry(&applied_list, struct payload,
>+                                                   applied_list);

const

>@@ -1260,6 +1363,9 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
>*action)
     >case XSPLICE_ACTION_APPLY:
         >if ( data->state == XSPLICE_STATE_CHECKED )
         >{
>+            rc = build_id_dep(data, list_empty(&applied_list));

!!list_empty()

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel