diff mbox

[v5,1/4] livepatch/docs: Document .bss not being cleared, and .data potentially having changed values

Message ID 20160913155919.GA3326@char.us.oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Sept. 13, 2016, 3:59 p.m. UTC 
On Mon, Sep 12, 2016 at 01:49:51AM -0600, Jan Beulich wrote:
> >>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
> > --- a/docs/misc/livepatch.markdown
> > +++ b/docs/misc/livepatch.markdown
> > @@ -875,6 +875,12 @@ section and the new function will reference the new string in the new
> >  
> >  This is implemented in the Xen Project hypervisor.
> >  
> > +Note that the .bss section is only cleared when the ELF payload is uploaded.
> > +Subsequent apply/revert/apply operation do no clear the .bss (or reset the
> > +.data to what it was when loaded). Hence it is the responsibility of the
> > +creator of the payload to reset these values to known good state if they
> > +depend on them having certain values at apply/revert states.
> 
> Was it, as an alternative, considered to disallow re-applying a
> reverted patch without re-uploading?

I was thinking about it but not exactly sure of the ramifications.

That is if you go this route - revert a patch - we could go the
next step and just unload it. But that puts some question on how
to schedule that without corruption - say we have payload A,B and we
are replacing A,B with C. A,B should be reverted and unloaded..

That means some form of list .. Ugh.

We could do a simpler way (which is what I think inline with your
suggestion). This does the trick (and needs to be split up obviously)
and also handles the case where you only have .text changes (like
for NOPs).

Interestingly, the linker always adds an .bss and .data section
even if the code does not produce it.

Comments

Jan Beulich Sept. 13, 2016, 4:12 p.m. UTC | #1 
>>> On 13.09.16 at 17:59, <konrad.wilk@oracle.com> wrote:
> On Mon, Sep 12, 2016 at 01:49:51AM -0600, Jan Beulich wrote:
>> >>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
>> > --- a/docs/misc/livepatch.markdown
>> > +++ b/docs/misc/livepatch.markdown
>> > @@ -875,6 +875,12 @@ section and the new function will reference the new string in the new
>> >  
>> >  This is implemented in the Xen Project hypervisor.
>> >  
>> > +Note that the .bss section is only cleared when the ELF payload is uploaded.
>> > +Subsequent apply/revert/apply operation do no clear the .bss (or reset the
>> > +.data to what it was when loaded). Hence it is the responsibility of the
>> > +creator of the payload to reset these values to known good state if they
>> > +depend on them having certain values at apply/revert states.
>> 
>> Was it, as an alternative, considered to disallow re-applying a
>> reverted patch without re-uploading?
> 
> I was thinking about it but not exactly sure of the ramifications.
> 
> That is if you go this route - revert a patch - we could go the
> next step and just unload it. But that puts some question on how
> to schedule that without corruption - say we have payload A,B and we
> are replacing A,B with C. A,B should be reverted and unloaded..
> 
> That means some form of list .. Ugh.
> 
> We could do a simpler way (which is what I think inline with your
> suggestion). This does the trick (and needs to be split up obviously)
> and also handles the case where you only have .text changes (like
> for NOPs).

At least that draft patch looks reasonable at a first glance.

Jan
diff mbox

Patch

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d9eeab1..4a1abb2 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -53,6 +53,8 @@  struct livepatch_build_id {
 struct payload {
     uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
     int32_t rc;                          /* 0 or -XEN_EXX. */
+    bool_t reverted;                     /* Whether it was reverted. */
+    bool_t safe_to_reapply;              /* Can apply safely after revert. */
     struct list_head list;               /* Linked to 'payload_list'. */
     const void *text_addr;               /* Virtual address of .text. */
     size_t text_size;                    /* .. and its size. */
@@ -313,7 +315,7 @@  static void calc_section(const struct livepatch_elf_sec *sec, size_t *size,
 static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 {
     void *text_buf, *ro_buf, *rw_buf;
-    unsigned int i;
+    unsigned int i, rw_buf_sec, rw_buf_cnt = 0;
     size_t size = 0;
     unsigned int *offset;
     int rc = 0;
@@ -386,7 +388,11 @@  static int move_payload(struct payload *payload, struct livepatch_elf *elf)
             if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
                 buf = text_buf;
             else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
+            {
                 buf = rw_buf;
+                rw_buf_sec = i;
+                rw_buf_cnt++;
+            }
             else
                 buf = ro_buf;
 
@@ -407,6 +413,11 @@  static int move_payload(struct payload *payload, struct livepatch_elf *elf)
         }
     }
 
+    if ( rw_buf_cnt == 1 )
+    {
+        if ( !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+            payload->safe_to_reapply = true;
+    }
  out:
     xfree(offset);
 
@@ -1120,6 +1131,7 @@  static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    data->reverted = true;
     return 0;
 }
 
@@ -1501,6 +1513,19 @@  static int livepatch_action(xen_sysctl_livepatch_action_t *action)
     case LIVEPATCH_ACTION_APPLY:
         if ( data->state == LIVEPATCH_STATE_CHECKED )
         {
+            /*
+             * It is unsafe to apply an reverted as the .data may not be in
+             * in pristine condition. Hence MUST unload and then apply patch
+             * again.
+             */
+            if ( data->reverted && !data->safe_to_reapply )
+            {
+                dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. Please unload!\n",
+                        LIVEPATCH, data->name);
+                data->rc = -EINVAL;
+                break;
+            }
+
             rc = build_id_dep(data, !!list_empty(&applied_list));
             if ( rc )
                 break;
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 69b0cdd..e03a26d 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -72,7 +72,7 @@  $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
 note.o:
 	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin
 	$(OBJCOPY) $(OBJCOPY_MAGIC) \
-		   --rename-section=.data=.livepatch.depends -S $@.bin $@
+		   --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
 	rm -f $@.bin
 
 #
@@ -83,7 +83,7 @@  note.o:
 hello_world_note.o: $(LIVEPATCH)
 	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin
 	$(OBJCOPY) $(OBJCOPY_MAGIC) \
-		   --rename-section=.data=.livepatch.depends -S $@.bin $@
+		   --rename-section=.data=.livepatch.depends,,alloc,load,readonly,data,contents -S $@.bin $@
 	rm -f $@.bin
 
 xen_bye_world.o: config.h
@@ -103,6 +103,8 @@  xen_nop.o: config.h
 .PHONY: $(LIVEPATCH_NOP)
 $(LIVEPATCH_NOP): xen_nop.o note.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+	$(OBJCOPY) --remove-section=.bss --remove-section=.data $@
+	$(OBJCOPY) --strip-unneeded $@
 
 .PHONY: livepatch
 livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)