from:"Wieczorkiewicz, Pawel"



> On 16. Sep 2019, at 19:54, Ross Lagerwall  wrote:
> 
>> 
snip
>> 2) post-apply hook
>>   runs after the apply action has been executed and quiescing zone
>>   exited. Its main purpose is to provide an ability to follow-up on
>>   actions performed by the pre- hook, when module application was
>>   successful or undo certain preparation steps of the pre- hook in
>>   case of a failure. The success/failure error code is proviVded to
> 
> provided

ACK.

> 
>>   the post- hooks via the rc field of the payload structure.
>> 3) pre-revert hook
>>   runs before the revert action is scheduled for execution. Its main
>>   purpose is to prevent from reverting a hotpatch when certain
> 
> Let's stick with "livepatch" terminology to avoid confusion (throughout this 
> patch).
> 

ACK.

>>   expected conditions aren't met or when mutating actions implemented
>>   in the hook fail or cannot be executed.
>> 4) post-revert hook
>>   runs after the revert action has been executed and quiescing zone
>>   exited. Its main purpose is to perform cleanup of all previously
>>   executed mutating actions in order to restore the original system
>>   state from before the current module application.
>>   The success/failure error code is provided to the post- hooks via
>>   the rc field of the payload structure.
> 
> snip
> 
>> +/*
>> + * Check if payload has any of the vetoing, non-atomic hooks assigned.
>> + * A vetoing, non-atmic hook may perform an operation that changes the
>> + * hypervisor state and may not be guaranteed to succeed. Result of
>> + * such operation may be returned and may change the livepatch workflow.
>> + * Such hooks may require additional cleanup actions performed by other
>> + * hooks. Thus they are not suitable for replace action.
>> + */
>> +static inline bool_t has_payload_any_vetoing_hooks(const struct payload 
>> *payload)
> 
> Use bool instead (throughout this patch).

ACK.

> 
>> +{
>> +return is_hook_enabled(payload->hooks.apply.pre) ||
>> +   is_hook_enabled(payload->hooks.apply.post) ||
>> +   is_hook_enabled(payload->hooks.revert.pre) ||
>> +   is_hook_enabled(payload->hooks.revert.post);
>> +}
>> +
>> +/*
>> + * Checks if any of the already applied hotpatches has any vetoing,
>> + * non-atomic hooks assigned.
>> + */
> snip
> 
>> @@ -1560,6 +1681,30 @@ static int livepatch_action(struct 
>> xen_sysctl_livepatch_action *action)
>>  rc = build_id_dep(data, 1 /* against hypervisor. */);
>>  if ( rc )
>>  break;
>> +
>> +/*
>> + * REPLACE action is not supported on hotpatches with vetoing 
>> hooks.
>> + * Vetoing hooks usually perform mutating actions on the system 
>> and
>> + * typically exist in pairs (pre- hook doing an action and 
>> post- hook
>> + * undoing the action). Coalescing all hooks from all applied 
>> modules
>> + * cannot be performed without inspecting potential 
>> dependencies between
>> + * the mutating hooks and hence cannot be performed 
>> automatically by
>> + * the replace action. Also, the replace action cannot safely 
>> assume a
>> + * successful revert of all the module with vetoing hooks. When 
>> one
>> + * of the hooks fails due to not meeting certain conditions the 
>> whole
>> + * replace operation must have been reverted with all previous 
>> pre- and
>> + * post- hooks re-executed (which cannot be guaranteed to 
>> succeed).
>> + * The simplest response to this complication is disallow 
>> replace
>> + * action on modules with vetoing hooks.
>> + */
> 
> I think that allowing pre-apply veto hooks would be useful for the replace 
> action so the live patch can check if the system is in a good state before 
> doing the replace (this would certainly be useful for XenServer). It would be 
> safe as far as I can see with the caveat that it can't mutate state. But this 
> doesn't have to be done now.

Yes, I agree that allowing pre-apply hook makes sense for replace action and 
could be pretty useful.
Especially, that it runs before quiescing the CPUs, so any mutating actions 
must be chosen with care anyway.

Let’s create another patch for this, after the current patchset is merged.

> 
>> +if ( has_payload_any_vetoing_hooks(data) || 
>> livepatch_applied_have_vetoing_hooks() )
>> +{
>> +printk(XENLOG_ERR LIVEPATCH "%s: REPLACE action is not 
>> supported on hotpatches with vetoing hooks!\n",
>> +   data->name);
>> +rc = -EOPNOTSUPP;
>> +break;
>> +}
>> +
>>  data->rc = -EAGAIN;
>>  rc = schedule_work(data, action->cmd, action->timeout);
>>  }
>> diff --git a/xen/include/xen/livepatch_payload.h 
>> b/xen/include/xen/livepatch_payload.h
>> index 99613af2db..cd20944cc4 100644
>> ---

Re: [Xen-devel] [PATCH v3 10/12] livepatch: Handle arbitrary size names with the list operation



> On 17. Sep 2019, at 10:48, Jan Beulich  wrote:
> 
> On 17.09.2019 10:40,  Wieczorkiewicz, Pawel  wrote:
>> 
>> 
>> On 17. Sep 2019, at 10:27, Jan Beulich 
>> mailto:jbeul...@suse.com>> wrote:
>> 
>> On 16.09.2019 12:59, Pawel Wieczorkiewicz wrote:
>> @@ -951,11 +952,13 @@ struct xen_sysctl_livepatch_list {
>>   amount of payloads and version.
>>   OUT: How many payloads left. */
>>uint32_t pad;   /* IN: Must be zero. */
>> +uint64_t name_total_size;   /* OUT: Total size of all 
>> transfer names */
>> 
>> Why uint64_t and not uint32_t? You don't expect this to grow
>> beyond 4GiB, do you?
>> 
>> I don’t, but uint32_t is not really compatible with size_t.
>> And I was thought to always use size_t compatible types for sizes.
> 
> That's a fair point, but I think we use 32-bit sizes elsewhere
> as well, when crossing the 4GiB boundary would seem entirely
> unexpected.
> 
> But what's worse here - you shouldn't use plain uint64_t in
> sysctl.h (and domctl.h) anyway. If anything, you ought to use
> uint64_aligned_t. (Going through the file I notice a few other
> bad instances have crept in.)
> 

I see. Noted, thanks for letting me know.

>> Anyway, I do not mind changing this to whatever type you prefer.
> 
> Well, preference - if anyone's - would be the livepatch maintainers'
> one here.
> 

Waiting for the maintainers' wise judgment then :-).

> Also - can you please see about adjusting your reply style? In plain
> text mode it's impossible to tell context from your responses.

Oh, sorry about that. I tweaked my settings.
Please let me know if it does not get better.

> 
> Jan

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 10/12] livepatch: Handle arbitrary size names with the list operation



On 17. Sep 2019, at 10:27, Jan Beulich 
mailto:jbeul...@suse.com>> wrote:

On 16.09.2019 12:59, Pawel Wieczorkiewicz wrote:
@@ -951,11 +952,13 @@ struct xen_sysctl_livepatch_list {
   amount of payloads and version.
   OUT: How many payloads left. */
uint32_t pad;   /* IN: Must be zero. */
+uint64_t name_total_size;   /* OUT: Total size of all transfer 
names */

Why uint64_t and not uint32_t? You don't expect this to grow
beyond 4GiB, do you?

I don’t, but uint32_t is not really compatible with size_t.
And I was thought to always use size_t compatible types for sizes.

Anyway, I do not mind changing this to whatever type you prefer.


And why OUT rather than IN/OUT? Once you make this "arbitrary
size", I don't see a need for limiting this to ...

XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status;  /* OUT. Must have 
enough
   space allocate for nr of them. */
XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each member
-   MUST XEN_LIVEPATCH_NAME_SIZE in 
size.
-   Must have nr of them. */
+   may have an arbitrary length up 
to
+   XEN_LIVEPATCH_NAME_SIZE bytes. 
Must have
+   nr of them. */

... XEN_LIVEPATCH_NAME_SIZE bytes per entry.

Changing the upper bound limitation will break certain assumptions and I did 
not want to pile all these on top of the current change.
But, yes, the upper bound limit could be dropped. I would prefer to do it as an 
independent patch.


And finally - please send to the list just once, i.e. please
don't have two xen-devel@ in the recipients list.


Sorry, I did not notice the add_maintainers.pl script adds an explicit To: for 
the xen-devel@.

Jan

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 02/12] livepatch: Allow to override inter-modules buildid dependency



On 16. Sep 2019, at 19:01, Ross Lagerwall 
mailto:ross.lagerw...@citrix.com>> wrote:

On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
snip
+/*
+ * Parse user provided action flags.
+ * This function expects to only receive an array of input parameters being 
flags.
+ * Expected action is specified via idx paramater (index of flag_options[]).
+ */
+static int get_flags(int argc, char *argv[], unsigned int idx, uint64_t *flags)
+{
+int i, j;
+
+if ( !flags || idx >= ARRAY_SIZE(flag_options) )
+return -1;
+
+*flags = 0;
+for ( i = 0; i < argc; i++ )
+{
+for ( j = 0; j < ARRAY_SIZE(flag_options[idx]); j++ )
+{
+if ( !flag_options[idx][j].name )
+goto error;
+
+if ( !strcmp(flag_options[idx][j].name, argv[i]) )
+{
+*flags |= flag_options[idx][j].flag;
+break;
+}
+}
+
+if ( j == ARRAY_SIZE(flag_options[idx]) )
+goto error;
+}
+
+return 0;
+error:
+fprintf(stderr, "Unsupported flag: %s.\n", argv[i]);
+errno = EINVAL;
+return errno;
+}

You return -1 above but +ve errno here. Please make it consistent.

Well, I understood from the code of the file (e.g. action_func()) that the -1 
value indicates a unexpected runtime error (negative val).
Whereas, positive errno values are expected error to be dealt with.

So:
<0 - fatal errors
0 - ok
>0 - errors to be handled

Could you confirm please that I should make get_flags() return only positive 
errors?

Also, you don't need to set errno if returning the actual error.


Honestly, I just copied the code from get_name() and wanted to the get_flags() 
to follow similar pattern.

(The error handling in this file looks fairly inconsistent anyway but let's not 
make it worse.)

+
 /* The hypervisor timeout for the live patching operation is 30 msec,
  * but it could take some time for the operation to start, so wait twice
  * that period. */
@@ -291,8 +357,9 @@ int action_func(int argc, char *argv[], unsigned int idx)
 char name[XEN_LIVEPATCH_NAME_SIZE];
 int rc;
 xen_livepatch_status_t status;
+uint64_t flags;
 -if ( argc != 1 )
+if ( argc < 1 )
 {
 show_help();
 return -1;
@@ -301,7 +368,10 @@ int action_func(int argc, char *argv[], unsigned int idx)
 if ( idx >= ARRAY_SIZE(action_options) )
 return -1;
 -if ( get_name(argc, argv, name) )
+if ( get_name(argc--, argv++, name) )
+return EINVAL;
+
+if ( get_flags(argc, argv, idx, ) )
 return EINVAL;
   /* Check initial status. */
@@ -332,7 +402,7 @@ int action_func(int argc, char *argv[], unsigned int idx)
 if ( action_options[idx].allow & status.state )
 {
 printf("%s %s... ", action_options[idx].verb, name);
-rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS);
+rc = action_options[idx].function(xch, name, HYPERVISOR_TIMEOUT_NS, 
flags);
 if ( rc )
 {
 int saved_errno = errno;
@@ -394,17 +464,23 @@ int action_func(int argc, char *argv[], unsigned int idx)
   static int load_func(int argc, char *argv[])
 {
-int rc;
-char *new_argv[2];
-char *path, *name, *lastdot;
+int i, rc = ENOMEM;
+char *upload_argv[2];
+char **apply_argv, *path, *name, *lastdot;
 -if ( argc != 1 )
+if ( argc < 1 )
 {
 show_help();
 return -1;
 }
+
+/* apply action has  [flags] input requirement, which must be 
constructed */
+apply_argv = (char **) malloc(argc * sizeof(*apply_argv));
+if ( !apply_argv )
+return rc;
+
 /*  */
-new_argv[1] = argv[0];
+upload_argv[1] = argv[0];
   /* Synthesize the  */
 path = strdup(argv[0]);
@@ -413,16 +489,23 @@ static int load_func(int argc, char *argv[])
 lastdot = strrchr(name, '.');
 if ( lastdot != NULL )
 *lastdot = '\0';
-new_argv[0] = name;
+upload_argv[0] = name;
+apply_argv[0] = name;
 -rc = upload_func(2 /*   */, new_argv);
+/* Fill in all user provided flags */
+for ( i = 0; i < argc - 1; i++ )
+apply_argv[i + 1] = argv[i + 1];

Wouldn't this make the loop body simpler?  i = 1; i < argc;


ACK. It would indeed.

Or alternatively, just a straight memcpy().

--
Ross Lagerwall

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 01/12] livepatch: Always check hypervisor build ID upon hotpatch upload



On 16. Sep 2019, at 18:23, Ross Lagerwall 
mailto:ross.lagerw...@citrix.com>> wrote:

On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
This change is part of a independant stacked hotpatch modules
feature. This feature allows to bypass dependencies between modules
upon loading, but still verifies Xen build ID matching.
In order to prevent (up)loading any hotpatches built for different
hypervisor version as indicated by the Xen Build ID, add checking for
the payload's vs Xen's build id match.
To achieve that embed into every hotpatch another section with a
dedicated hypervisor build id in it. After the payload is loaded and
the .livepatch.xen_depends section becomes available, perform the
check and reject the payload if there is no match.
snip
+sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
+if ( sec )
+{
+n = sec->load_addr;
+
+if ( sec->sec->sh_size <= sizeof(*n) )
+return -EINVAL;
+
+if ( xen_build_id_check(n, sec->sec->sh_size,
+>xen_dep.p, >xen_dep.len) )
+return -EINVAL;
+
+if ( !payload->xen_dep.len || !payload->xen_dep.p )
+return -EINVAL;
+}
+
 /* Setup the virtual region with proper data. */
 region = >region;
 @@ -882,6 +922,10 @@ static int load_payload_data(struct payload *payload, 
void *raw, size_t len)
 if ( rc )
 goto out;
 +rc = check_xen_build_id(payload);
+if ( rc )
+goto out;
+
 rc = build_symbol_table(payload, );
 if ( rc )
 goto out;

It is a bit confusing having a new function called check_xen_build_id() when 
there is already a xen_build_id_check(). Perhaps the new one should be called 
xen_build_id_dep() as it is analogous to the existing build_id_dep()?


Yes, that definitely makes sense. I will squash it into v4. Otherwise, I hope 
it can be fixed upon merging.

Either way,

Reviewed-by: Ross Lagerwall 
mailto:ross.lagerw...@citrix.com>>

Many thanks!

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations

2019-08-29 Thread Wieczorkiewicz, Pawel



On 29. Aug 2019, at 19:49, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com>> wrote:

On Thu, Aug 29, 2019 at 04:16:13PM +, Wieczorkiewicz, Pawel wrote:


On 29. Aug 2019, at 17:58, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com><mailto:konrad.w...@oracle.com>>
 wrote:



…snip..


Could you please try with the patch attached?

It compiled, but the test-case was not happy. See attached the full serial log


Ah, I forgot about endianness of course...
I am sending an improved patch. I hope it works this time as expected.





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879





If it still fails, could you please send me the output for the following
command with this build?

objdump -d xen-syms | sed -n -e '/:$/,/^$/ p' | tail -n +2

Also included in the serial log.


Best Regards,
Pawel Wieczorkiewicz








Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




0001-fixup-livepatch-Add-support-for-inline-asm-hotpatchi.patch
Description: 0001-fixup-livepatch-Add-support-for-inline-asm-hotpatchi.patch

Best Regards,Pawel Wieczorkiewicz

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations

2019-08-29 Thread Wieczorkiewicz, Pawel



On 29. Aug 2019, at 17:58, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com>> wrote:

+CODE_GET_EXPECT=$(shell objdump -d --insn-width=1 $(1) | grep -A6 -E 
'<'$(2)'>:' | tail -n +2 | awk 'BEGIN {printf "{"} {printf "0x%s,", $$2}' | sed 
's/,$$/}/g')

Ony my Hikey 960 when I compile using an native compiler I get:

gcc  -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin 
-fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ 
-include /home/xen.git/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_expectations.o"' -Wa,--strip-local-absolute -g -MMD -MF 
./.xen_expectations.o.d -mcpu=generic -mgeneral-regs-only   
-I/home/xen.git/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE  -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_expectations.c -o xen_expectations.o
/home/xen.git/xen/Rules.mk:202: recipe for target 'xen_expectations.o' failed
make[3]: Circular expect_config.h <- xen_expectations.o dependency dropped.
In file included from xen_expectations.c:6:0:
expect_config.h:1:23: error: large integer implicitly truncated to unsigned type
[-Werror=overflow]
#define EXPECT_BYTES {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
c09112e0}
  ^
xen_expectations.c:28:17: note: in expansion of macro ‘EXPECT_BYTES’
.data = EXPECT_BYTES
^~~~
expect_config.h:1:34: error: large integer implicitly truncated to unsigned type
[-Werror=overflow]
#define EXPECT_BYTES {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
c09112e0}
 ^
xen_expectations.c:28:17: note: in expansion of macro ‘EXPECT_BYTES’
.data = EXPECT_BYTES
^~~~
expect_config.h:1:45: error: large integer implicitly truncated to unsigned type
[-Werror=overflow]
#define EXPECT_BYTES {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
c09112e0}
…

make[3]: Leaving directory '/home/xen.git/xen/test/livepatch'
Makefile:11: recipe for target 'build' failed
make[2]: Leaving directory '/home/xen.git/xen/test'
Makefile:85: recipe for target '_tests' failed
make[1]: Leaving directory '/home/xen.git/xen'
Makefile:45: recipe for target 'tests' failed
root@hikey960:/home/xen.git/xen# cat test/livepatch/expect_config.h
#define EXPECT_BYTES 
{0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0xc09112e0}
#define EXPECT_BYTES_COUNT 6



Could you please try with the patch attached?




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




0001-fixup-livepatch-Add-support-for-inline-asm-hotpatchi.patch
Description: 0001-fixup-livepatch-Add-support-for-inline-asm-hotpatchi.patch
If it still fails, could you please send me the output for the following command with this build?objdump -d xen-syms | sed -n -e '/:$/,/^$/ p' | tail -n +2
Best Regards,Pawel Wieczorkiewicz

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations

2019-08-29 Thread Wieczorkiewicz, Pawel



On 29. Aug 2019, at 16:34, Konrad Rzeszutek Wilk 
mailto:kon...@darnok.org>> wrote:



…snip...

+
+struct livepatch_func __section(".livepatch.funcs") livepatch_exceptions = {
+.version = LIVEPATCH_PAYLOAD_VERSION,
+.name = livepatch_exceptions_str,
+.new_addr = xen_hello_world,
+.old_addr = xen_extra_version,
+.new_size = EXPECT_BYTES_COUNT,
+.old_size = EXPECT_BYTES_COUNT,
+.expect = {
+.enabled = 1,
+.len = EXPECT_BYTES_COUNT,
+.data = EXPECT_BYTES
+},
+
+};

When I compile with 32-bit ARM 'make tests' I get:

arm-eabi-ld-EL -EL --build-id=sha1 -r -o 
xen_action_hooks_norevert.livepatch xen_action_hooks_marker.o 
xen_hello_world_func.o note.o xen_note.o
make[3]: Circular expect_config.h <- xen_expectations.o dependency dropped.
objdump: can't disassemble for architecture UNKNOWN!

(set -e; \
echo "#define EXPECT_BYTES {"; \
echo "#define EXPECT_BYTES_COUNT 6") > expect_config.h
arm-eabi-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall 
-Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin 
-fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ 
-include /home/konrad/A20/xen.git/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_expectations.o"' -Wa,--strip-local-absolute -g -MMD -MF 
./.xen_expectations.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-8250.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x01c28000 -DEARLY_UART_REG_SHIFT=2  
-I/home/konrad/A20/xen.git/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_expectations.c -o xen_expectations.o
xen_expectations.c:31:2: error: expected '}' before ';' token
};
 ^
xen_expectations.c:18:76: note: to match this '{'
struct livepatch_func __section(".livepatch.funcs") livepatch_exceptions = {
   ^
make[3]: *** [/home/konrad/A20/xen.git/xen/Rules.mk:202: xen_expectations.o] 
Error 1
make[3]: Leaving directory '/home/konrad/A20/xen.git/xen/test/livepatch'


And this is what expect_config.h looks like:

#define EXPECT_BYTES {
#define EXPECT_BYTES_COUNT 6

Hi Konrad,

I think I fixed it with the following change on top:

[DIFF START]
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 067861903f..20bdeb6c6d 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -186,10 +186,17 @@ xen_actions_hooks_norevert.o: config.h
 $(LIVEPATCH_ACTION_HOOKS_NOREVERT): xen_action_hooks_marker.o 
xen_hello_world_func.o note.o xen_note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o 
$(LIVEPATCH_ACTION_HOOKS_NOREVERT) $^

-CODE_GET_EXPECT=$(shell objdump -d --insn-width=1 $(1) | grep -A6 -E 
'<'$(2)'>:' | tail -n +2 | awk 'BEGIN {printf "{"} {printf "0x%s,", $$2}' | sed 
's/,$$/}/g')
+ifeq ($(findstring arm, $(TARGET_ARCH) $(ARCH)),arm)
+INSN_WIDTH := 4
+INSN_PER_LINE := 2
+else
+INSN_WIDTH := 1
+INSN_PER_LINE := 8
+endif
+CODE_GET_EXPECT=$(shell $(OBJDUMP) -d --insn-width=$(INSN_WIDTH) $(1) | sed -n 
-e '/<'$(2)'>:$$/,/^$$/ p' | tail -n +2 | head -n $(INSN_PER_LINE) | awk 
'{printf "%s", $$2}' | sed 's/.\{2\}/0x&,/g' | sed 's/^/{/;s/,$$/}/g')
 .PHONY: expect_config.h
 expect_config.h: EXPECT_BYTES=$(call 
CODE_GET_EXPECT,$(BASEDIR)/xen-syms,xen_extra_version)
-expect_config.h: EXPECT_BYTES_COUNT=6
+expect_config.h: EXPECT_BYTES_COUNT=8
 expect_config.h: xen_expectations.o
(set -e; \
 echo "#define EXPECT_BYTES $(EXPECT_BYTES)"; \
[DIFF END]

I will test that some more and add it to the v3 of the patchset.

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 05/12] livepatch: Add support for apply|revert action replacement hooks

2019-08-28 Thread Wieczorkiewicz, Pawel



On 27. Aug 2019, at 18:58, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com>> wrote:

On August 27, 2019 4:46:17 AM EDT, Pawel Wieczorkiewicz 
mailto:wipa...@amazon.de>> wrote:
By default, in the quiescing zone, a hotpatch payload is applied with
apply_payload() and reverted with revert_payload() functions. Both of
the functions receive the payload struct pointer as a parameter. The
functions are also a place where standard 'load' and 'unload' module
hooks are executed.

To increase hotpatching system's agility and provide more flexiable
long-term hotpatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.

Is there sense in having the old ones anymore? We could just remove them 
altogether and just use the new ones instead from the start?


Yes, I believe there is. The old ones represent well-tested, proven way of
payload application (or revert) which preserves conservative behavior of the 
system
via enforcing certain assumptions.
The old ones cover 99% of the production use cases, so I would insist on keeping
them in (at least for now).

Later, we could modify livepatch-build-tools to always generate the original
apply|revert functions (in a default case) as part of payloads (hooks).
But, I would not do it just now.

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker


On 22. Aug 2019, at 17:30, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:

Hi Pawel,

On 22/08/2019 12:02, Wieczorkiewicz, Pawel wrote:
On 22. Aug 2019, at 12:29, Julien Grall 
mailto:julien.gr...@arm.com> 
<mailto:julien.gr...@arm.com>> wrote:
On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote:
More generally, I am not very comfortable to see panic() in the middle of the 
code. Could you explain why panic is the best solution over reverting the work?

Yes. Production-ready hotpatches must not contain inconsistent hooks or 
functions-to-be-applied/reverted.
The goal here is to detect such hotpatches and fail hard immediately 
highlighting the fact that such hotpatch
is broken.

Aside the len = 0 that you are going to fix. How would this condition happen? 
Are you going to add code that will potentially trigger the panic?


The default livepatch code path is very conservative (to the extent it does not 
even need this check and panic).
But, with the changes of this series, one can potentially construct a hotpatch 
that upon load would trigger the panic
(or without the panic, would silently leave the Xen code in memory in an 
inconsistent state). This obviously must not
happen in production, so the new livepatch feature should be used with care.The 
changes make livepatch more
flexible and universal, yet when using new features, more care is needed.

However, when someone is developing a hotpatch or is using the new feature for 
debugging or for whatever
non-production-related reason, it is very helpful to detect immediately any 
sort of undefined state.
I have been there myself when I was working on the changes presented here, and 
thought that would be a good idea
to add this checks permanently.
Also, when something changes in the future in the livepatch implementation, 
those checks could potentially highlight
any inconsistencies.

The inconsistent application of a hotpatch (some function applied, some 
reverted while other left behined) leaves
the system in a very bad state. I think the best what we could do here is 
panic() to enable post-mortem analysis
of what went wrong and avoid leaking such system into production.

Thank you for the explanation here (and on IRC). May I ask some documentation 
regarding the panic in at least commit message? Ideally, this would explain why 
the panic the most sensible solution.


Yes, certainly. I will add that.

Cheers,

--
Julien Grall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker

On 22. Aug 2019, at 12:43, Julien Grall
mailto:julien.gr...@arm.com>> wrote:

Hi,

On 22/08/2019 11:20, Wieczorkiewicz, Pawel wrote:

..snip..

Cross-compiler are nowadays widely available. So build testing your changes in
common code would be the minimum.

I wish it was that simple. Nevertheless, I will try to prepare an environment
to perform such builds.

Cross-compiling the hypervisor is really easy ;).

1) Download the cross-compiler tarball (here one [1]) and uncompress it. You
can also install the one provided by your distro.

2) Build Xen hypervisor. Here an example for arm64:

42sh> cd xen.git/xen
42sh> make XEN_TARGET_ARCH=arm64 CROSS_COMPILER=-

In my case, I am using the Arm toolchain AArch64 GNU/Linux target
(aarch64-linux-gnu). So the would be aarch64-linux-gnu.

This is assuming you have the compilers binary in your PATH. If not, you can
use give the full path:

CROSS_COMPILER=/opt/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-

Awesome! That really works (especially thanks for the [1] link… finally some
toolstack that works on my system).

One change was needed: s/CROSS_COMPILER/CROSS_COMPILE/g

Thanks!

Having this in a wiki would really help. Or have I missed it?

In this case, as you dropped the const from the prototype, you will need to do
the same in the declaration.

Yes, but I see 2 options here:
- Enable the feature also for Arm (I prefer that, but will not be able to test
that in nearest future)

I think some of the code can be made common. So we could possibly rely on x86
for that. Additionally, IIRC, Konrad has a setup on the cubietruck for testing
livepatch.

Yes, I will do that.

- Keep Arm excluded and sprinkle code with #ifdef CONFIG_X86

Please no #ifdef CONFIG_X86 in the common code. If you don't plan to support
Arm, then we should introduce a new Kconfig that will gate all those changes.

Ugh, you’re right. Removing all that from common code.

Cheers,

[1]
https://developer.arm.com/tools-and-software/open-source-software/developer-tools/gnu-toolchain/gnu-a/downloads

--
Julien Grall

Best Regards,
Pawel Wieczorkiewicz

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 10/14] livepatch: Add support for inline asm hotpatching expectations



On 22. Aug 2019, at 12:31, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:

Hi Pawel,

On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote:
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index b55ad6d050..e18322350d 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -818,7 +818,7 @@ struct xen_sysctl_cpu_featureset {
  * If zero exit with success.
  */
 -#define LIVEPATCH_PAYLOAD_VERSION 2
+#define LIVEPATCH_PAYLOAD_VERSION 3

We usually only bump the version once per release. So this is unnecessary as 
you already did it in the previous patch.


Yes, I will keep all these changes under a single bump as promised to Konrad 
yesterday.

Cheers,

--
Julien Grall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker



On 22. Aug 2019, at 12:29, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:

Hi Pawel,

Hi Julien,

Thank for looking at this!


On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote:
Livepatch only tracks an entire payload applied/reverted state. But,
with an option to supply the apply_payload() and/or revert_payload()
functions as optional hooks, it becomes possible to intermix the
execution of the original apply_payload()/revert_payload() functions
with their dynamically supplied counterparts.
It is important then to track the current state of every function
being patched and prevent situations of unintentional double-apply
or unapplied revert.
To support that, it is necessary to extend public interface of the
livepatch. The struct livepatch_func gets additional field holding
the applied/reverted state marker.
To reflect the livepatch payload ABI change, bump the version flag
LIVEPATCH_PAYLOAD_VERSION up to 2.
The above solution only applies to x86 architecture for now.
Signed-off-by: Pawel Wieczorkiewicz 
mailto:wipa...@amazon.de>>
Reviewed-by: Andra-Irina Paraschiv 
mailto:andra...@amazon.com>>
Reviewed-by: Bjoern Doebel mailto:doe...@amazon.de>>
Reviewed-by: Martin Pohlack mailto:mpohl...@amazon.de>>
---
 xen/arch/x86/livepatch.c| 20 +++-
 xen/common/livepatch.c  | 35 +++
 xen/include/public/sysctl.h | 11 ++-
 xen/include/xen/livepatch.h |  2 +-
 4 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 436ee40fe1..76fa91a082 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -61,6 +61,14 @@ void noinline arch_livepatch_apply(struct livepatch_func 
*func)
 if ( !len )
 return;
 +/* If the apply action has been already executed on this function, do 
nothing... */
+if ( func->applied == LIVEPATCH_FUNC_APPLIED )
+{
+printk(XENLOG_WARNING LIVEPATCH "%s: %s has been already applied 
before\n",
+__func__, func->name);
+return;
+}

Does this need to in arch specific code?

As a matter of fact it does not.
Let me enable this feature for Arm as well and make this code a common inline.


+
 memcpy(func->opaque, old_ptr, len);
 if ( func->new_addr )
 {
@@ -77,15 +85,25 @@ void noinline arch_livepatch_apply(struct livepatch_func 
*func)
 add_nops(insn, len);
   memcpy(old_ptr, insn, len);
+func->applied = LIVEPATCH_FUNC_APPLIED;
 }
   /*
  * "noinline" to cause control flow change and thus invalidate I$ and
  * cause refetch after modification.
  */
-void noinline arch_livepatch_revert(const struct livepatch_func *func)
+void noinline arch_livepatch_revert(struct livepatch_func *func)
 {
+/* If the apply action hasn't been executed on this function, do 
nothing... */
+if ( !func->old_addr || func->applied == LIVEPATCH_FUNC_NOT_APPLIED )
+{
+printk(XENLOG_WARNING LIVEPATCH "%s: %s has not been applied before\n",
+__func__, func->name);
+return;
+}
+
 memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
+func->applied = LIVEPATCH_FUNC_NOT_APPLIED;
 }
   /*
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 585ec9819a..090a48977b 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1242,6 +1242,29 @@ static inline void revert_payload_tail(struct payload 
*data)
 data->state = LIVEPATCH_STATE_CHECKED;
 }
 +/*
+ * Check if an action has applied the same state to all payload's functions 
consistently.
+ */
+static inline bool was_action_consistent(const struct payload *data, 
livepatch_func_state_t expected_state)
+{
+int i;
+
+for ( i = 0; i < data->nfuncs; i++ )
+{
+struct livepatch_func *f = &(data->funcs[i]);
+
+if ( f->applied != expected_state )

Per the definition of livepath_func, the field "applied" only exists for x86. 
So this will not compiled on Arm.

I will enable Arm too.


+{
+printk(XENLOG_ERR LIVEPATCH "%s: Payload has a function: '%s' with 
inconsistent applied state.\n",
+   data->name, f->name ?: "noname");
+
+return false;
+}
+}
+
+return true;
+}
+
 /*
  * This function is executed having all other CPUs with no deep stack (we may
  * have cpu_idle on it) and IRQs disabled.
@@ -1268,6 +1291,9 @@ static void livepatch_do_action(void)
 else
 rc = apply_payload(data);
 +if ( !was_action_consistent(data, rc ? LIVEPATCH_FUNC_NOT_APPLIED : 
LIVEPATCH_FUNC_APPLIED) )

Regardless the compilation issue above, none of the common code will set the 
state. So for Arm, this would likely mean the function return false.

True, I should have disabled this code for Arm too.
But now, let me just make it common for all archs.


+panic("livepatch: partially applied payload '%s'!\n", data->name);

I can see at least a case where the panic can be reached

Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker


On 22. Aug 2019, at 12:07, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:

Hi,

On 22/08/2019 08:44, Wieczorkiewicz, Pawel wrote:


…snip...


Is this going to break livepatch on Arm? If so, do you have any plan to fix it?

No, I do not think it is. But, I am unable to test on Arm (No access to HW and 
SW),
so I took the conservative approach here.
Arm provides decent free model (see FoundationModel) that you can use for basic 
testing. Alternatively, you QEMU also support virtualization extension.

Let me have a look at the code (I will answer separately) to see if I can spot 
anything.

[...]

diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 2aec532ee2..a93126f631 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -117,7 +117,7 @@ int arch_livepatch_quiesce(void);
 void arch_livepatch_revive(void);
   void arch_livepatch_apply(struct livepatch_func *func);
-void arch_livepatch_revert(const struct livepatch_func *func);
+void arch_livepatch_revert(struct livepatch_func *func);

I doubt livepatch on Arm will compile after this change.

What would be you suggestion then?

Cross-compiler are nowadays widely available. So build testing your changes in 
common code would be the minimum.


I wish it was that simple. Nevertheless, I will try to prepare an environment 
to perform such builds.

In this case, as you dropped the const from the prototype, you will need to do 
the same in the declaration.


Yes, but I see 2 options here:
- Enable the feature also for Arm (I prefer that, but will not be able to test 
that in nearest future)
- Keep Arm excluded and sprinkle code with #ifdef CONFIG_X86

Shall I limit the change to X86 everywhere
 Or maybe drop the compilation flag completely?

I am a bit confused. Which compilation flag do you refer to?


CONFIG_X86

Cheers,

--
Julien Grall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker


On 21. Aug 2019, at 23:34, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:

Hi Pawel,

Hi Julien,


On 8/21/19 9:19 AM, Pawel Wieczorkiewicz wrote:
Livepatch only tracks an entire payload applied/reverted state. But,
with an option to supply the apply_payload() and/or revert_payload()
functions as optional hooks, it becomes possible to intermix the
execution of the original apply_payload()/revert_payload() functions
with their dynamically supplied counterparts.
It is important then to track the current state of every function
being patched and prevent situations of unintentional double-apply
or unapplied revert.
To support that, it is necessary to extend public interface of the
livepatch. The struct livepatch_func gets additional field holding
the applied/reverted state marker.
To reflect the livepatch payload ABI change, bump the version flag
LIVEPATCH_PAYLOAD_VERSION up to 2.
The above solution only applies to x86 architecture for now.

Is this going to break livepatch on Arm? If so, do you have any plan to fix it?


No, I do not think it is. But, I am unable to test on Arm (No access to HW and 
SW),
so I took the conservative approach here.

[...]

diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 2aec532ee2..a93126f631 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -117,7 +117,7 @@ int arch_livepatch_quiesce(void);
 void arch_livepatch_revive(void);
   void arch_livepatch_apply(struct livepatch_func *func);
-void arch_livepatch_revert(const struct livepatch_func *func);
+void arch_livepatch_revert(struct livepatch_func *func);

I doubt livepatch on Arm will compile after this change.


What would be you suggestion then?
Shall I limit the change to X86 everywhere
 Or maybe drop the compilation flag completely?

 void arch_livepatch_post_action(void);
   void arch_livepatch_mask(void);

Cheers,

--
Julien Grall

Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/20] livepatch-build: detect special section group sizes

> On 21. Aug 2019, at 20:49, Konrad Rzeszutek Wilk  
> wrote:
> 
> 
>> +# Using xen-syms built in the previous step by build_full().
>> +SPECIAL_VARS=$(readelf -wi "$OUTPUT/xen-syms" |
> 
> What version of readelf supports this? Asking as in the past there were some 
> options with binutils that had conflicting options and we had to add some 
> custom hackery code to figure out which parameter to use.

The version I use does: GNU readelf version 2.23.52.0.1 20130226
The -wi is a shortcut for —debug-dump=info.
I do hope it’s a standard readelf feature by now.

Best Regards,
Pawel Wieczorkiewicz

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 06/14] livepatch: Add support for apply|revert action replacement hooks


On 21. Aug 2019, at 20:31, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com>> wrote:

On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote:
By default, in the quiescing zone, a hotpatch payload is applied with
apply_payload() and reverted with revert_payload() functions. Both of
the functions receive the payload struct pointer as a parameter. The
functions are also a place where standard 'load' and 'unload' module
hooks are executed.
To increase hotpatching system's agility and provide more flexiable
long-term hotpatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.
Since the alternative functions have direct access to the hotpatch
payload structure, they can better control context of the 'load' and
'unload' hooks execution as well as exact instructions replacement
workflows. They can be also easily extended to support extra features
in the future.
To simplify the alternative function generation move code responsible
for payload and hotpatch region registration outside of the function.
That way it is guaranteed that the registration step occurs even for
newly supplied functions.

This logic looks legit, but I was wondering if you would be up for a writing a 
small test-case(s) livepatch that would exercise these parts just to make sure 
nothing in the future would screw it up?

Yes, I could do that. But, I would like to discuss (get guidelines about) the 
expected test coverage.
With this sort of changes, there is an unlimited set of test-cases to be 
created. So, I would like to focus on a few most important.
I am missing knowledge how these test cases are supposed to be used… hence the 
ask.

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 10/14] livepatch: Add support for inline asm hotpatching expectations


> On 21. Aug 2019, at 20:30, Konrad Rzeszutek Wilk  
> wrote:
> 
> On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote:
>>  typedef enum livepatch_func_state {
>>  LIVEPATCH_FUNC_NOT_APPLIED = 0,
>>  LIVEPATCH_FUNC_APPLIED = 1
>> @@ -838,11 +850,12 @@ struct livepatch_func {
>>  uint32_t new_size;
>>  uint32_t old_size;
>>  uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>> -uint8_t opaque[31];
>> +uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
>>  #if defined CONFIG_X86
>>  uint8_t applied;
>>  uint8_t _pad[7];
>>  #endif
>> +livepatch_expectation_t expect;
>>  };
> 
> Aaah, now I understand why you decide to create a new field _pad and applied 
> field!
> 

Yup, that was premeditated! :-)

> Any particular reason why the version can't be 2 since this can be part of 
> this changeset? Also you would need to have a Documentation change.

It surely can be 2. I wasn’t sure before if the changes could go together. Will 
change to 2 for both

And, I will also update the doc. 

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker


> On 21. Aug 2019, at 20:28, Konrad Rzeszutek Wilk  
> wrote:
> 
> On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote:
>>  struct livepatch_func {
>>  const char *name;   /* Name of function to be patched. */
>>  void *new_addr;
>> @@ -834,6 +839,10 @@ struct livepatch_func {
>>  uint32_t old_size;
>>  uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>>  uint8_t opaque[31];
>> +#if defined CONFIG_X86
>> +uint8_t applied;
>> +uint8_t _pad[7];
>> +#endif
> 
> Replying here as well. Could you use part of the 'opaque[31]' space instead 
> for this field?


No, as explained earlier, I must not do that. The opaque[] is not a padding.

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-hooks-2 PATCH 4/4] livepatch: Add per-function applied/reverted state tracking marker


> On 21. Aug 2019, at 20:12, Konrad Rzeszutek Wilk  
> wrote:
> 
> On 8/14/19 4:39 AM, Pawel Wieczorkiewicz wrote:
>> #ifdef __XEN__
>> +typedef enum livepatch_func_state {
>> +LIVEPATCH_FUNC_NOT_APPLIED = 0,
>> +LIVEPATCH_FUNC_APPLIED = 1
>> +} livepatch_func_state_t;
>> +
>>  struct livepatch_func {
>>  const char *name;   /* Name of function to be patched. */
>>  void *new_addr;
>> @@ -834,6 +839,10 @@ struct livepatch_func {
>>  uint32_t old_size;
>>  uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
>>  uint8_t opaque[31];
>> +#if defined CONFIG_X86
>> +uint8_t applied;
>> +uint8_t _pad[7];
>> +#endif
>>  };
> 
> Three requests:
>  - Why does it have to be X86 specific?

Mostly because I am unable to test that on ARM (ENODEV…).

>  - Can you also include the change in the documentation where the spec 
> resides?

Yes, will do.

>  - You are bumping the version to 2, but not making it backwards 
> compatible. If so ,you also need to update the documentation to mention 
> this.


Ok, I will update that as well.

Best Regards,
Pawel Wieczorkiewicz




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-hooks-2 PATCH 2/4] create-diff-object: Add support for applied/reverted marker


On 21. Aug 2019, at 20:09, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com>> wrote:

On 8/14/19 4:38 AM, Pawel Wieczorkiewicz wrote:
With version 2 of a payload structure additional field is supported
to track whether given function has been applied or reverted.
There also comes additional 8-byte alignment padding to reserve
place for future flags and options.

The new fields are zero-out upon .livepatch.funcs section creation.

Signed-off-by: Pawel Wieczorkiewicz 
mailto:wipa...@amazon.de>>
---
 common.h | 2 ++
 create-diff-object.c | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/common.h b/common.h
index 06e19e7..d8cde35 100644
--- a/common.h
+++ b/common.h
@@ -124,6 +124,8 @@ struct livepatch_patch_func {
  uint32_t old_size;
  uint8_t version;
  unsigned char pad[31];

So the 31 pad is for this purpose - that you can make it smaller. Why
not use that?

No, I must not use that. The 31 pad should be actually called opaque,
and corresponds to the location where hypervisor stores replaced bytes
of the replaced functions.


+ uint8_t applied;
+ uint8_t _pad[7];
 };

 struct special_section {
diff --git a/create-diff-object.c b/create-diff-object.c
index 263c7d2..534516b 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -2009,8 +2009,10 @@ static void livepatch_create_patches_sections(struct 
kpatch_elf *kelf,
  funcs[index].old_size = result.size;
  funcs[index].new_addr = 0;
  funcs[index].new_size = sym->sym.st_size;
- funcs[index].version = 1;
+ funcs[index].version = 2;
  memset(funcs[index].pad, 0, sizeof funcs[index].pad);
+ funcs[index].applied = 0;
+ memset(funcs[index]._pad, 0, sizeof funcs[index]._pad);

  /*
   * Add a relocation that will populate

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-hooks-2 PATCH 3/4] livepatch: Add support for apply|revert action replacement hooks


On 21. Aug 2019, at 20:10, Konrad Rzeszutek Wilk 
mailto:konrad.w...@oracle.com>> wrote:

On 8/14/19 4:38 AM, Pawel Wieczorkiewicz wrote:
By default, in the quiescing zone, a hotpatch payload is applied with
apply_payload() and reverted with revert_payload() functions. Both of
the functions receive the payload struct pointer as a parameter. The
functions are also a place where standard 'load' and 'unload' module
hooks are executed.

To increase hotpatching system's agility and provide more flexiable
long-term hotpatch solution, allow to overwrite the default apply
and revert action functions with hook-like supplied alternatives.
The alternative functions are optional and the default functions are
used by default.

Since the alternative functions have direct access to the hotpatch
payload structure, they can better control context of the 'load' and
'unload' hooks execution as well as exact instructions replacement
workflows. They can be also easily extended to support extra features
in the future.

To simplify the alternative function generation move code responsible
for payload and hotpatch region registration outside of the function.
That way it is guaranteed that the registration step occurs even for
newly supplied functions.


You MUST also include the test-cases for this new functionality.

Yeah, I will add them. How comprehensive tests do you have in mind?
I would add 1 or 2 general cases for both of the new hooks. Is it good enough?

Please add them, you know where they are right?

I think, I do.

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch: independ. modules v2 2/3] livepatch: Allow to override inter-modules buildid dependency


> On 20. Aug 2019, at 16:28, Julien Grall  wrote:
> 
> Hi,
> 
>>> 
…snip...
>> Yeah, since I got feedback and reviews on various patches that I have 
>> already submitted the way I did,
>> I simply continue with what I have until all comments are addressed (I do 
>> not want to lose anything).
> 
> What do you mean by "all comments are addressed"? Usually you gather a set of 
> comments for a series, address them and then resend the series with all of 
> them addressed.
> 

Yeah, but people invested time in reviews and replied to my incorrectly sent 
threads, so I find it rude to ignore such comments and start a new thread 
instead.
But if this is the recommended practice, I will obey.

>> Then, I will re-send the patches in 2 series: livepatch-build-tools and xen 
>> with all changes,
>> Reviewed-by/Acked-by and cover letters. This is the way recommended by 
>> Andrew.
> 
> Please don't send the patch one by one to check if everyone is happy. Just 
> resend all of them in one go once you gathered enough feedback.
> 

Ok.

>> Unfortunately, it will be also quite confusing I think, because various 
>> changes belonging to different topics,
>> are distributed between those 2 distinct repos.
> 
> That also happen when you have multiple patches in a series. Feature 
> implemented accross multiple patch needs a place to discuss. This can usually 
> be done in the cover letter. For multi repo series, you can steer the 
> discussion on a single repo and just replicate the changes in the other one 
> once there are an agreement.

Fine. Let me then send the 2 full series for each repo with all the changes and 
corresponding cover letters.

> 
> Cheers,
> 
> -- 
> Julien Grall


Best Regards,
Pawel Wieczorkiewicz




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch: independ. modules v2 2/3] livepatch: Allow to override inter-modules buildid dependency


> On 20. Aug 2019, at 15:35, Julien Grall  wrote:
> 
> Hi,
> 
> Something looks fishy in the threading:
> 
>  - The patch #1 is answered in reply-to the patch #1 of version 1.
>  - This patch (#2) is answered in reply-to the patch #2 of version 1.
>  - The patch #3 is labeled as v3 an in reply-to the patch #3 of version 1.
> 
> If you send them as series, then they should be sent together for a new 
> version and in a new thread. Not mangled to the previous thread as this makes 
> quite difficult to follow what's going on.
> 
> Also it looks like the series is still lacking of the cover letter. So I 
> still have no clue what "livepatch: independ. modules" in your [...] refers 
> to.
> 

Yeah, since I got feedback and reviews on various patches that I have already 
submitted the way I did,
I simply continue with what I have until all comments are addressed (I do not 
want to lose anything).

Then, I will re-send the patches in 2 series: livepatch-build-tools and xen 
with all changes,
Reviewed-by/Acked-by and cover letters. This is the way recommended by Andrew.

Unfortunately, it will be also quite confusing I think, because various changes 
belonging to different topics,
are distributed between those 2 distinct repos. 

Best,
Pawel

> Cheers,
> 
> On 20/08/2019 14:28, Pawel Wieczorkiewicz wrote:
>> By default Livepatch enforces the following buildid-based dependency
>> chain between hotpatch modules:
>>   1) first module depends on given hypervisor buildid
>>   2) every consecutive module depends on previous module's buildid
>> This way proper hotpatch stack order is maintained and enforced.
>> While it is important for production hotpatches it limits agility and
>> blocks usage of testing or debug hotpatches. These kinds of hotpatch
>> modules are typically expected to be loaded at any time irrespective
>> of current state of the modules stack.
>> To enable testing and debug hotpatches allow user dynamically ignore
>> the inter-modules dependency. In this case only hypervisor buildid
>> match is verified and enforced.
>> To allow userland pass additional paremeters for livepatch actions
>> add support for action flags.
>> Each of the apply, revert, unload and revert action gets additional
>> 64-bit parameter 'flags' where extra flags can be applied in a mask
>> form.
>> Initially only one flag '--nodeps' is added for the apply action.
>> This flag modifies the default buildid dependency check as described
>> above.
>> The global sysctl interface input flag parameter is defined with a
>> single corresponding flag macro:
>>   LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
>> The userland xen-livepatch tool is modified to support the '--nodeps'
>> flag for apply and load commands. A general mechanism for specifying
>> more flags in the future for apply and other action is however added.
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Eslam Elnikety 
>> Reviewed-by: Petre Eftime 
>> Reviewed-by: Leonard Foerster 
>> Reviewed-by: Martin Pohlack 
>> Reviewed-by: Norbert Manthey 
>> ---
>> v2:
>> * Bump XEN_SYSCTL_INTERFACE_VERSION to 0x0013
>> ---
>>  tools/libxc/include/xenctrl.h |   9 ++--
>>  tools/libxc/xc_misc.c |  20 +++
>>  tools/misc/xen-livepatch.c| 121 
>> +++---
>>  xen/common/livepatch.c|  14 +++--
>>  xen/include/public/sysctl.h   |  11 +++-
>>  5 files changed, 139 insertions(+), 36 deletions(-)
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 0ff6ed9e70..725697c132 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2607,11 +2607,12 @@ int xc_livepatch_list(xc_interface *xch, unsigned 
>> int max, unsigned int start,
>>   * to complete them. The `timeout` offers an option to expire the
>>   * operation if it could not be completed within the specified time
>>   * (in ns). Value of 0 means let hypervisor decide the best timeout.
>> + * The `flags` allows to pass extra parameters to the actions.
>>   */
>> -int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout);
>> -int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout);
>> -int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout);
>> -int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout);
>> +int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout, 
>> uint64_t flags);
>> +int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout, 
>> uint64_t flags);
>> +int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout, 
>> uint64_t flags);
>> +int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, 
>> uint64_t flags);
>>/*
>>   * Ensure cache coherency after memory modifications. A call to this 
>> function
>> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
>> index 8e60b6e9f0..a8e9e7d1e2 100644
>> --- a/tools/libxc/xc_misc.c

Re: [Xen-devel] [PATCH livepatch-python 1/1] livepatch: Add python bindings for livepatch operations

On 19. Aug 2019, at 23:39, Marek Marczykowski-Górecki 
mailto:marma...@invisiblethingslab.com>> wrote:

On Thu, Aug 15, 2019 at 11:36:46AM +, Pawel Wieczorkiewicz wrote:
Extend the XC python bindings library to support also all common
livepatch operations and actions.


…snip...
+
+static PyObject *pyxc_livepatch_action(XcObject *self,
+   PyObject *args,
+   PyObject *kwds)
+{
+int (*action_func)(xc_interface *xch, char *name, uint32_t timeout, 
uint64_t flags);

This makes it dependent on "livepatch: Allow to override inter-modules
buildid dependency" patch. Since it's part of a different series, if
there is going to be v2, please name the dependency explicitly in the
commit message or at least after —.

ACK. Will do.

+char *name;
+unsigned int action;
+uint32_t timeout;
+uint64_t flags;
+int rc;
+

…snip...
+static PyObject *pyxc_livepatch_upload(XcObject *self,
+   PyObject *args,
+   PyObject *kwds)
+{
+unsigned char *fbuf = MAP_FAILED;
+char *name, *filename;
+struct stat buf;
+int fd = 0, rc;
+ssize_t len;
+
+static char *kwd_list[] = { "name", "filename", NULL };
+
+if ( !PyArg_ParseTupleAndKeywords(args, kwds, "ss", kwd_list,
+  , ))

It would be nice to support Path-like objects on python >= 3.6. See
note about "s" format here:
https://docs.python.org/3/c-api/arg.html#strings-and-buffers

But since it's python 3 only, that would need to be under #if
PY_MAJOR_VERSION >= 3.


I’d prefer to add it as a separate commit (adding it to my TODO).

+goto error;
+
+fd = open(filename, O_RDONLY);
+if ( fd < 0 )
+goto error;
+
+if ( stat(filename, ) != 0 )
+goto error;
+

…snip...
+
+rc = xc_livepatch_list_get_sizes(self->xc_handle, ,
+ _total_size, _total_size);

This makes it dependent on lp-metadata series. Same note as previously.
BTW for some reason your patch series are not handled as a mail threads,
which makes it harder to look for other patches in the series. Is it
only me?


ACK, Will do.

No, unfortunately it’s me, who incorrectly sent out the series. I divided them
by functionality instead of per repo. I also did not create a cover letter. But,
I got more than one advice how to do it next time.

+if ( rc )
+goto error;
+

…snip...
+
+list = PyList_New(0);

Better use PyList_New(done) and later PyList_SetItem() instead of 
PyList_Append().


ACK. Will change.

+name_off = metadata_off = 0;
+for ( i = 0; i < done; i++ )
+{
+PyObject *info_dict, *metadata_list;
+char *name_str, *metadata_str;
+
…snip...
(m, "LIVEPATCH_STATE_CHECKED", LIVEPATCH_STATE_CHECKED);
+
#if PY_MAJOR_VERSION >= 3
  return m;
#endif

--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH livepatch-python 1/1] livepatch: Add python bindings for livepatch operations





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




PGPMIME Versions Identification
Description: PGP/MIME Versions Identification


encrypted.asc
Description: OpenPGP encrypted message.asc
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch: independ. modules 3/3] python: Add XC binding for Xen build ID


> On 19. Aug 2019, at 22:40, Marek Marczykowski-Górecki 
>  wrote:
> 
> On Thu, Aug 15, 2019 at 09:44:00AM +, Pawel Wieczorkiewicz wrote:
>> Extend the list of xc() object methods with additional one to display
>> Xen's buildid. The implementation follows the libxl implementation
>> (e.g. max buildid size assumption being XC_PAGE_SIZE).
>> 
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Martin Mazein 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Norbert Manthey 
>> ---
>> v2:
>> 
…snip...
>> 
>> +static PyObject *pyxc_xenbuildid(XcObject *self)
>> +{
>> +xen_build_id_t *buildid;
>> +int i, r;
>> +char *str;
>> +
>> +buildid = alloca(sizeof(buildid->len) + XC_PAGE_SIZE);
>> +buildid->len = XC_PAGE_SIZE - sizeof(*buildid);
> 
> Those doesn't match. You allocated XC_PAGE_SIZE in addition to
> sizeof(buildid->len). I'd change to alloca(XC_PAGE_SIZE) - it is
> unlikely that izeof(buildid->len) would be larger than XC_PAGE_SIZE and
> we do assume it in other places anyway.

ACK. Will fix.

> 
>> +
>> +r = xc_version(self->xc_handle, XENVER_build_id, buildid);
>> +if ( r <= 0 )
>> +return pyxc_error_to_exception(self->xc_handle);
>> +
>> 

…snip...

> 
> -- 
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?


Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part3 v2 3/3] create-diff-object: Strip all undefined entires of known size

2019-08-19 Thread Wieczorkiewicz, Pawel


On 16. Aug 2019, at 17:02, Ross Lagerwall 
mailto:ross.lagerw...@citrix.com>> wrote:

On 8/8/19 1:51 PM, Pawel Wieczorkiewicz wrote:
The patched ELF object file contains all sections and symbols as
resulted from the compilation. However, certain symbols may not be
copied over to the resulting object file, due to being unchanged or
not included for other reasons.
In such situation the resulting object file has the entire sections
copied along (with all their entries unchanged), while some of the
corresponding symbols are not copied along at all.
This leads to having incorrect undefined (STN_UNDEF) entries in the
final hotpatch ELF file.
The newly added function livepatch_strip_undefined_elements() detects
and removes all undefined RELA entries as well as their corresponding
PROGBITS section entries.
Since the sections may contain elements of unknown size (sh.sh_entsize
== 0), perform the strip only on sections with well defined entry
sizes.
After replacing the stripped rela list, it is assumed that the next
invocation of the kpatch_rebuild_rela_section_data() will adjust all
section header parameters according to the current state.

The code in this patch seems to be very similar (i.e. somewhat copy-and-pasted) 
to kpatch_regenerate_special_section() which prunes the rela list and rebuilds 
the corresponding text section according to the predicate 
should_keep_rela_group(). The intent of the function also seems to be the same 
(only keep elements that are needed).

In what situations does the existing function not do the right thing?

Can should_keep_rela_group() be updated instead?


Yes, the code is largely based on the kpatch_regenerate_special_section(). 
However, the livepatch_strip_undefined_elements() and 
kpatch_regenerate_special_section() have different "granularity" and different 
scope.
1. Scope
- livepatch_strip_undefined_elements(): all RELA sections
- kpatch_regenerate_special_section(): special RELA sections

2. "granularity"
- livepatch_strip_undefined_elements(): all relas to be checked
- kpatch_regenerate_special_section(): all relas in a group are copied

I think it should be possible to create a common function with the base 
functionality that could take most of the common code in and then be used from 
within both of the functions.
However, I did not want to touch existing functionality and I am afraid the 
changes may result in a pretty convoluted code.

If you think it is still worth it, I can give it a shot.

Thanks,
--
Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH livepatch-python 1/1] livepatch: Add python bindings for livepatch operations

> On 16. Aug 2019, at 14:37, Wei Liu  wrote:
> 
> On Thu, Aug 15, 2019 at 11:36:46AM +, Pawel Wieczorkiewicz wrote:
>> Extend the XC python bindings library to support also all common
>> livepatch operations and actions.
>> 
>> 

…snip...

>> 
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Martin Mazein 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Leonard Foerster 
>> Reviewed-by: Norbert Manthey 
> 
> I haven't looked in details, but I'm fine with these new functionalities
> in general. Let's see if Marek has any objections.

Thanks.

> 
> Which version of Python do you use to build these? The requirement here
> is the code should build with both Python 2.5 and Python 3.
> 

Ah, I see. Thanks for pointing this out. We’re planing to upstream the tool 
using those bindings as well.
But, it still requires some work. Let us add the python versions support for 
the tool.

> Wei.

Best Regards,
Pawel Wieczorkiewicz

signature.asc
Description: Message signed with OpenPGP

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH lp-metadata 3/3] livepatch: Add metadata runtime retrieval mechanism


> On 16. Aug 2019, at 14:44, Wei Liu  wrote:
> 
> On Thu, Aug 15, 2019 at 11:27:50AM +, Pawel Wieczorkiewicz wrote:
>> Extend the livepatch list operation to fetch also payloads' metadata.
>> This is achieved by extending the sysctl list interface with 2 extra
>> guest handles:
>> * metadata - an array of arbitrary size strings
>> * metadata_len - an array of metadata strings' lengths (uin32_t each)

…snip...

>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Martin Pohlack 
>> Reviewed-by: Norbert Manthey 
>> ---
>> tools/libxc/include/xenctrl.h | 22 +++
>> tools/libxc/xc_misc.c | 66 
>> +++
>> tools/misc/xen-livepatch.c| 43 ++--
>> xen/common/livepatch.c| 22 +++
>> xen/include/public/sysctl.h   | 19 +
> 
> Mostly look good. One comment below...
> 
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index b86804b7a6..e4c8f4fe63 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
> 
> If it hasn't been done for this release already, changing sysctl interface 
> requires
> bumping the version number in this header.
> 

ACK. Will do (also for earlier patches…).

> Wei.

Best Regards,
Pawel Wieczorkiewicz


signature.asc
Description: Message signed with OpenPGP



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch: independ. modules 3/3] python: Add XC binding for Xen build ID


On 16. Aug 2019, at 14:47, Wei Liu mailto:w...@xen.org>> wrote:

On Thu, Aug 15, 2019 at 09:44:00AM +, Pawel Wieczorkiewicz wrote:
Extend the list of xc() object methods with additional one to display
Xen's buildid. The implementation follows the libxl implementation
(e.g. max buildid size assumption being XC_PAGE_SIZE).

Signed-off-by: Pawel Wieczorkiewicz 
mailto:wipa...@amazon.de>>
Reviewed-by: Martin Mazein mailto:amaz...@amazon.de>>
Reviewed-by: Andra-Irina Paraschiv 
mailto:andra...@amazon.com>>
Reviewed-by: Norbert Manthey mailto:nmant...@amazon.de>>

I'm a bit confused by the tag in the subject line. Which series does
this patch belong to?

Wei.

Thanks for taking a look.

This is the series: https://marc.info/?t=15554198232=1=4

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part2 v2 6/6] create-diff-object: Do not include all .rodata sections


> On 16. Aug 2019, at 11:57, Ross Lagerwall  wrote:
> 
> On 8/8/19 1:39 PM, Pawel Wieczorkiewicz wrote:
>> 

…snip...

>>  #define inc_printf(fmt, ...) \
>>  log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__);
> This patch looks good. There is a comment at the top of 
> should_include_str_section() which should probably be updated as well:
> 
> /*
> * String sections are always included even if unchanged.
> * The format is either:
> * .rodata..str1.[0-9]+ (new in GCC 6.1.0)
> * or .rodata.str1.[0-9]+ (older versions of GCC)
> * For the new format we could be smarter and only include the needed
> * strings sections.
> */
> 

Oh yes, right. Let me update the comment. Thanks!

> In fact, it is probably a good idea to rename the function to something like 
> "is_rodata_str_section()" since this more accurately describes what it does 
> now.

ACK, will do.

> 
> Thanks,
> --
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz





signature.asc
Description: Message signed with OpenPGP



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part2 v2 5/6] create-diff-object: Add new entries to special sections array array


On 16. Aug 2019, at 11:40, Ross Lagerwall 
mailto:ross.lagerw...@citrix.com>> wrote:

On 8/8/19 1:35 PM, Pawel Wieczorkiewicz wrote:


…snip...

  * The rela groups in the .fixup section vary in size.  The beginning of each
  * .fixup rela group is referenced by the .ex_table section. To find the size
@@ -1072,6 +1090,18 @@ static struct special_section special_sections[] = {
  .name = ".altinstructions",
  .group_size = altinstructions_group_size,
  },
+ {
+ .name = ".altinstr_replacement",
+ .group_size = undefined_group_size,
+ },
+ {
+ .name = ".livepatch.hooks.load",
+ .group_size = livepatch_hooks_group_size,
+ },
+ {
+ .name = ".livepatch.hooks.unload",
+ .group_size = livepatch_hooks_group_size,
+ },
  {},
 };


Unless I'm misunderstanding something, I can't see how 
kpatch_regenerate_special_section would work with .altinstr_replacement having 
a group size of 0. It looks to me like the for loop in that function would 
become an infinite loop (due to incrementing by group_size) and 
should_keep_rela_group would always return false.


AFAICS, the group_size 0 sections are never actually processed by the 
kpatch_regenerate_special_section().
They are not RELA sections and the following check excludes them from this 
processing:

static void kpatch_process_special_sections(struct kpatch_elf *kelf)
{
[…]
for (special = special_sections; special->name; special++) {
[…]

sec = sec->rela;
if (!sec)
continue;

kpatch_regenerate_special_section(kelf, special, sec);
}

Nevertheless, you are right. It does not make much sense to rely on this 
assumption.
I will add explicit checks to kpatch_regenerate_special_section() and friends 
dealing with group_size 0 sections.

Regards,
--
Ross Lagerwall

Best,
Pawel Wieczorkiewicz




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH lp-metadata 2/3] livepatch: Handle arbitrary size names with the list operation

2019-08-15 Thread Wieczorkiewicz, Pawel

Thanks Julien. I will do that next time (unless you guys want me to re-send all 
this ;-)).

BTW, I also pushed my changes onto the xenbits server:
http://xenbits.xenproject.org/gitweb/?p=people/wipawel/livepatch-build-tools;a=summary
http://xenbits.xenproject.org/gitweb/?p=people/wipawel/xen;a=summary

I hope that makes navigation and dealing with the swarm of patches a bit easier.

Best Regards,
Pawel Wieczorkiewicz



On 15. Aug 2019, at 17:29, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:



On 15/08/2019 16:19, Wieczorkiewicz, Pawel wrote:
Hi Lars, Julien,

Hi,

Thanks for the pointers, I will read them up and follow the recommendations 
with my future contributions.
Sorry for the mess…
But, let me ask first before reading the wikis, how do you prefer submitting 
series that contain patches belonging to 2 distinct repos (e.g. xen and 
livepatch-build-tools)?

I can see two ways:

 1) One series per project and mention in the cover letter that modifications 
are required in another project (with link/title).
 2) Combine all the patches in one series and tag them differently. I.e [XEN] 
[LIVEPATCH].

1) is preferable if you have a lot of patches in each repo. 2) can be handy if 
you have only a couple of patches for one repo.

Cheers,

Best Regards,
Pawel Wieczorkiewicz
On 15. Aug 2019, at 16:58, Lars Kurth 
mailto:lars.kurth@gmail.com> 
<mailto:lars.kurth@gmail.com>> wrote:



On 15 Aug 2019, at 12:38, Julien Grall 
mailto:julien.gr...@arm.com> 
<mailto:julien.gr...@arm.com>> wrote:

Hi,

I am not going to answer on the patch itself but the process.

Any series (i.e more than one patch) should contain a cover letter with a rough 
summary of the goal of the series.

Furthermore, this 3 patches series has been received as 3 separate threads (i.e 
in-reply-to is missing). This is making difficult to know that all the patches 
belongs to the same series. In general, all patches are send as in-reply-to the 
cover letter. So all the patches sticks together in one thread.

The cover letter can be generated via git format-patch --cover-letter. 
Threading is done automatically with git-send-email when all the patches as 
passed in arguments.

For more details how to do it, you can read:

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Sending_a_Patch_Series

Cheers,

Hi Pawel,

thank you for submitting the patch series.

We had a couple of new starters recently who followed a similar pattern to you. 
As a result of this, I recently updated the following docs

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches - Definitions 
and general workflow
The bit which saves the most work is 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Sending_a_Patch_Series
As for Julien's comment on the threading: see the --thread and --cover-letter 
option as described in the Sending a Patch Series

https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git - Basic Git 
commands fitting into the workflow, including how to deal with reviews
https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_StGit - Basic StGit 
commands fitting into the workflow, including how to deal with reviews
I have not had time to play with git series yet. If anyone in your team uses it 
let me know

In any case: if you follow the instructions the entire submission process and 
dealing with review comments becomes much easier.

As a newcomer, to contributing to Xen, I would greatly appreciate if you could 
let me know of any issues with the docs, such that we can fix them

Regards
Lars



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

--
Julien Grall




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH lp-metadata 2/3] livepatch: Handle arbitrary size names with the list operation

2019-08-15 Thread Wieczorkiewicz, Pawel

Hi Lars, Julien,

Thanks for the pointers, I will read them up and follow the recommendations 
with my future contributions.
Sorry for the mess…

But, let me ask first before reading the wikis, how do you prefer submitting 
series that contain patches belonging to 2 distinct repos (e.g. xen and 
livepatch-build-tools)?

Best Regards,
Pawel Wieczorkiewicz



On 15. Aug 2019, at 16:58, Lars Kurth 
mailto:lars.kurth@gmail.com>> wrote:



On 15 Aug 2019, at 12:38, Julien Grall 
mailto:julien.gr...@arm.com>> wrote:

Hi,

I am not going to answer on the patch itself but the process.

Any series (i.e more than one patch) should contain a cover letter with a rough 
summary of the goal of the series.

Furthermore, this 3 patches series has been received as 3 separate threads (i.e 
in-reply-to is missing). This is making difficult to know that all the patches 
belongs to the same series. In general, all patches are send as in-reply-to the 
cover letter. So all the patches sticks together in one thread.

The cover letter can be generated via git format-patch --cover-letter. 
Threading is done automatically with git-send-email when all the patches as 
passed in arguments.

For more details how to do it, you can read:

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Sending_a_Patch_Series

Cheers,

Hi Pawel,

thank you for submitting the patch series.

We had a couple of new starters recently who followed a similar pattern to you. 
As a result of this, I recently updated the following docs

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches - Definitions 
and general workflow
The bit which saves the most work is 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Sending_a_Patch_Series
As for Julien's comment on the threading: see the --thread and --cover-letter 
option as described in the Sending a Patch Series

https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git - Basic Git 
commands fitting into the workflow, including how to deal with reviews
https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_StGit - Basic StGit 
commands fitting into the workflow, including how to deal with reviews
I have not had time to play with git series yet. If anyone in your team uses it 
let me know

In any case: if you follow the instructions the entire submission process and 
dealing with review comments becomes much easier.

As a newcomer, to contributing to Xen, I would greatly appreciate if you could 
let me know of any issues with the docs, such that we can fix them

Regards
Lars







Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part2 6/6] create-diff-object: Do not include all .rodata sections

2019-04-30 Thread Wieczorkiewicz, Pawel


> On 29. Apr 2019, at 18:11, Ross Lagerwall  wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> Starting with the "2af6f1aa6233 Fix patch creation with GCC 6.1+" commit
>> all .rodata sections are included by default (regardless of whether they
>> are needed or not).
>> During stacked hotpatch builds it leads to unnecessary duplication of
>> the .rodata sections as each and every consecutive hotpatch contains all
>> the .rodata section of previous hotpatches.
> 
> This commit message is a bit confusing.
> 
> 1) All of this only applies to .rodata.str* sections. Other .rodata sections 
> are handled separately.

Yes, that’s right. I will make the commit message precise.

ACK. Will fix.

> 
> 2) The above commit _did not_ introduce the problem. Previous versions of GCC 
> did not split .rodata.str sections by function which meant that the entire 
> section was always included. The commit fixed patch creation and _maintained_ 
> the existing behaviour for GCC 6.1+ by including all the

I see, thanks for the clarification. I will update the wording to avoid 
confusion and incorrect attribution here.

> .rodata.str* sections. This patch now includes only what is needed (but it 
> should be noted that this would likely not be useful on older versions of GCC 
> since they don't split .rodata.str properly).

OK, I suppose users of older versions of GCC have to accept the fact.

> 
>> To prevent this situation, mark the .rodata section for inclusion only
>> if they are referenced by any of the current hotpatch symbols (or a
>> corresponding RELA section).
>> Extend patchability verification to detect all non-standard, non-rela,
>> non-debug and non-special sections that are not referenced by any of
>> the symbols or RELA sections.
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Bjoern Doebel 
>> Reviewed-by: Norbert Manthey 
>> ---
>>  create-diff-object.c | 27 ++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>> diff --git a/create-diff-object.c b/create-diff-object.c
>> index f6060cd..f7eb421 100644
>> --- a/create-diff-object.c
>> +++ b/create-diff-object.c
>> @@ -1341,7 +1341,7 @@ static void kpatch_include_standard_elements(struct 
>> kpatch_elf *kelf)
>>  list_for_each_entry(sec, >sections, list) {
>>  /* include these sections even if they haven't changed */
>> -if (is_standard_section(sec) || 
>> should_include_str_section(sec->name)) {
>> +if (is_standard_section(sec)) {
>>  sec->include = 1;
>>  if (sec->secsym)
>>  sec->secsym->include = 1;
>> @@ -1352,6 +1352,19 @@ static void kpatch_include_standard_elements(struct 
>> kpatch_elf *kelf)
>>  list_entry(kelf->symbols.next, struct symbol, list)->include = 1;
>>  }
>>  +static void kpatch_include_standard_string_elements(struct kpatch_elf 
>> *kelf)
>> +{
>> +struct section *sec;
>> +
>> +list_for_each_entry(sec, >sections, list) {
>> +if (should_include_str_section(sec->name) && 
>> is_referenced_section(sec, kelf)) {
>> +sec->include = 1;
>> +if (sec->secsym)
>> +sec->secsym->include = 1;
>> +}
>> +}
>> +}
> 
> I think it would be simpler to just amend the previous function rather than 
> introducing a new one. E.g.
> 
> ... || (should_include_str_section(sec->name) && is_referenced_section(sec, 
> kelf))

IIRC, it is unfortunately too early to do that from within 
kpatch_include_standard_elements().
I want to call the kpatch_include_standard_string_elements() after the 
following functions are called:
- kpatch_include_changed_functions()
- kpatch_include_debug_sections()
- kpatch_include_hook_elements()
because they may affect is_referenced_section() result.

> 
> Regards,
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part2 5/6] create-diff-object: Add new entries to special sections array

2019-04-30 Thread Wieczorkiewicz, Pawel


On 29. Apr 2019, at 17:47, Ross Lagerwall 
mailto:ross.lagerw...@citrix.com>> wrote:

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
Handle .livepatch.hooks* and .altinstr_replacement sections as the
special sections with assigned group_size resolution function.
By default each .livepatch.hooks* sections' entry is 8 bytes long (a
pointer). The .altinstr_replacement section follows the .altinstructions
section settings.
Allow to specify different .livepatch.hooks* section entry size using
shell environment variable HOOK_STRUCT_SIZE.
Cleanup an incorrect indentation around group_size resulution functions.
Signed-off-by: Pawel Wieczorkiewicz 
mailto:wipa...@amazon.de>>
Reviewed-by: Andra-Irina Paraschiv 
mailto:andra...@amazon.com>>
Reviewed-by: Bjoern Doebel mailto:doe...@amazon.de>>
Reviewed-by: Norbert Manthey mailto:nmant...@amazon.de>>
---
 create-diff-object.c | 87 
 1 file changed, 54 insertions(+), 33 deletions(-)
diff --git a/create-diff-object.c b/create-diff-object.c
index b0b4dcb..f6060cd 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -960,51 +960,64 @@ static void kpatch_mark_constant_labels_same(struct 
kpatch_elf *kelf)

Fix the indentation in the patch which introduces the problem rather than 
fixing it in a subsequent patch.

Oh, certainly. I forgot to update the commits involved properly.

ACK. Will fix.


   static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
 {
-static int size = 0;
-char *str;
-if (!size) {
-str = getenv("BUG_STRUCT_SIZE");
-size = str ? atoi(str) : 8;
-}
-
-return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("BUG_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ return size;
 }
   static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
 {
-static int size = 0;
-char *str;
-if (!size) {
-str = getenv("BUG_STRUCT_SIZE");
-size = str ? atoi(str) : 16;
-}
-
-return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("BUG_STRUCT_SIZE");
+ size = str ? atoi(str) : 16;
+ }
+
+ return size;
 }
   static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
 {
-static int size = 0;
-char *str;
-if (!size) {
-str = getenv("EX_STRUCT_SIZE");
-size = str ? atoi(str) : 8;
-}
-
-return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("EX_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ return size;
 }
   static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
 {
-static int size = 0;
-char *str;
-if (!size) {
-str = getenv("ALT_STRUCT_SIZE");
-size = str ? atoi(str) : 12;
-}
-
-printf("altinstr_size=%d\n", size);
-return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("ALT_STRUCT_SIZE");
+ size = str ? atoi(str) : 12;
+ }
+
+ printf("altinstr_size=%d\n", size);
+ return size;
+}
+
+static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
+{
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("HOOK_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ printf("livepatch_hooks_size=%d\n", size);

If you want to keep this debugging output, rather use log_debug().

ACK. Will fix.


+ return size;
 }
   /*
@@ -1084,6 +1097,14 @@ static struct special_section special_sections[] = {
  .name = ".altinstructions",
  .group_size = altinstructions_group_size,
  },
+ {
+ .name = ".altinstr_replacement",
+ .group_size = altinstructions_group_size,
+ },
+ {
+ .name = ".livepatch.hooks",
+ .group_size = livepatch_hooks_group_size,
+ },
  {},
 };


I don't understand the point of this change.

IIUC unlike .altinstructions, the .altinstr_replacement section does not have a 
fixed group size, so this change would not work other than by luck. If

That’s true. The .altinstr_replacement group_size should be 0 (aka undefined).

ACK. Will fix.

the goal was to prune bits of the .altinstr_replacement table that do not need 
to be included, you need to update the code in 
kpatch_process_special_sections() which has special handling for 
.altinstr_replacement. It needs to parse the updated, pruned .altinstructions 
section and regenerate .altinstr_replacement, including only what is needed.


No this was not the goal. I am explaining it below.

I don't know why .livepatch.hooks is included in this list either since all 
hooks are always included and there is nothing to regenerate/prune.

As discussed when reviewing [1], I need a function telling which section is 
special.
By special I mean:
a) should not be a subject of extra validation (See [2])
b) should not be a subject of STN_UNDEF purging (See [3])

Thus, I want to explicitly call out all "special" sections by their name and 
specify
their group_size whenever applicable (I did it incorrectly for 
.altinstr_replacement,
as you have spotted - thanks!) in

Re: [Xen-devel] [livepatch-build-tools part2 3/6] create-diff-object: Add is_special_section() helper function

2019-04-30 Thread Wieczorkiewicz, Pawel


> On 29. Apr 2019, at 17:25, Ross Lagerwall  wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> This function determines, based on the given section name, if the
>> sections belongs to the special sections category.
>> It treats a name from the special_sections array as a prefix. Any
>> sections whose name starts with special section prefix is considered
>> a special section.
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Bjoern Doebel 
>> Reviewed-by: Norbert Manthey 
>> ---
>>  common.c | 23 +++
>>  common.h |  1 +
>>  2 files changed, 24 insertions(+)
>> diff --git a/common.c b/common.c
>> index c968299..a2c860b 100644
>> --- a/common.c
>> +++ b/common.c
>> @@ -290,6 +290,29 @@ int is_referenced_section(const struct section *sec, 
>> const struct kpatch_elf *ke
>>  return false;
>>  }
>>  +int is_special_section(struct section *sec)
> 
> const for the parameter?

ACK. Will add.

> 
>> +{
>> +static struct special_section_names {
>> +const char *name;
>> +} names[] = {
>> +{ .name = ".bug_frames" },
>> +{ .name = ".fixup"  },
>> +{ .name = ".ex_table"   },
>> +{ .name = ".altinstructions"},
>> +{ .name = ".altinstr_replacement"   },
>> +{ .name = ".livepatch.hooks"},
>> +{ .name = NULL  },
>> +};
>> +struct special_section_names *special;
> 
> Wouldn't it be better to reuse the existing special_sections array rather 
> than partially duplicating it here?

After looking at this code again, I think you’re right and it would be better.
It would require to explicitly call out all sections to be considered special in
the special_sections array, but this is actually what I need for further 
changes anyway.

ACK. Will fix. 

> 
>> +
>> +for (special = names; special->name; special++) {
>> +if (!strncmp(sec->name, special->name, strlen(special->name)))
>> +return true;
> 
> This check is not as precise as it could be, since ".altinstructionsfoo" 
> would be considered special when it actually means nothing to this tool.

Given the above reasoning, that’s right.

ACK. Will fix.

> 
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part2 2/6] common: Add is_referenced_section() helper function


> On 29. Apr 2019, at 17:14, Ross Lagerwall  wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> This function checks if given section has an included corresponding
>> RELA section and/or any of the symbols table symbols references the
>> section. Section associated symbols are ignored here as there is
>> always such a symbol for every section.
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Bjoern Doebel 
>> Reviewed-by: Norbert Manthey 
>> ---
>>  common.c | 22 +-
>>  common.h |  1 +
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>> diff --git a/common.c b/common.c
>> index 1fb07cb..c968299 100644
>> --- a/common.c
>> +++ b/common.c
>> @@ -15,7 +15,7 @@
>>int is_rela_section(struct section *sec)
>>  {
>> -return (sec->sh.sh_type == SHT_RELA);
>> +return sec && (sec->sh.sh_type == SHT_RELA);
>>  }
>>int is_local_sym(struct symbol *sym)
>> @@ -270,6 +270,26 @@ int is_standard_section(struct section *sec)
>>  }
>>  }
>>  +int is_referenced_section(const struct section *sec, const struct 
>> kpatch_elf *kelf)
> 
> Let's keep to 80 chars where practical (and throughout the rest of the 
> patches).

ACK. Will fix. Although, sometimes it makes the code pretty unreadable.

> 
>> +{
>> +struct symbol *sym;
>> +
>> +if (is_rela_section(sec->rela) && sec->rela->include)
>> +return true;
>> +
>> +list_for_each_entry(sym, >symbols, list) {
>> +if (!sym->include || !sym->sec)
>> +continue;
>> +/* Ignore section associated sections */
>> +if (sym->type == STT_SECTION)
>> +continue;
>> +if (sym->sec->index == sec->index)
>> +return true;
> 
> You can simplify this check to `sym->sec == sec` like the rest of the code 
> does.

Might be my paranoia again (or I am simply missing some obvious 
assumptions/invariants),
but I explicitly wanted to check whether given section/symbol is referenced 
using other means
than addresses.

> 
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools 4/4] livepatch-build: Handle newly created object files


> On 29. Apr 2019, at 15:53, Ross Lagerwall  wrote:
> 
> On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
>> Up to now the livepatch-build ignores newly created object files.
>> When patch applies new .c file and augments its Makefile to build it
>> the resulting object file is not taken into account for final linking
>> step.
>> Such newly created object files can be detected by comparing patched/
>> and original/ directories and copied over to the output directory for
>> the final linking step.
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Andra-Irina Paraschiv 
>> Reviewed-by: Bjoern Doebel 
>> Reviewed-by: Norbert Manthey 
>> ---
>>  livepatch-build | 6 ++
>>  1 file changed, 6 insertions(+)
>> diff --git a/livepatch-build b/livepatch-build
>> index 796838c..b43730c 100755
>> --- a/livepatch-build
>> +++ b/livepatch-build
>> @@ -146,6 +146,12 @@ function create_patch()
>>  fi
>>  done
>>  +NEW_FILES=$(comm -23 <(find patched -type f -name '*.o' | cut -f2- 
>> -d'/' | sort -u) <(find original -type f -name '*.o' | cut -f2- -d'/' | sort 
>> -u))
> 
> The paths should be `patched/xen` and `original/xen` so that only hypervisor 
> changes are processed. It is done this way earlier (see FILES="$(find xen 
> ...)").

ACK. Will fix.

> 
> Since process substitution creates a subshell, it might be simpler to do <(cd 
> patched/xen && find . ...) and then drop the `cut`. Also, the `-u` argument 
> to sort seems unnecessary.

ACK. Will fix.
The -u for sort is just my paranoia.

> 
>> +for i in $NEW_FILES; do
>> +cp "patched/$i" "output/$i"
>> +CHANGED=1
>> +done
> 
> If the live patch for whatever reason has no "patched" object files and only 
> "new" object files, then it is not going to do anything useful since nothing 
> will use the new functions. This should fail to build with an error. E.g. set 
> NEW=1 above and then later check for CHANGED=0 && NEW=1.
> 

I disagree here. Inline assembly patching can use new functions even without 
having any "patched" objects.
Also hotpatch modules with hooks (functionality I will send upstream soon) can 
use the "new" objects provided
functionality for various reasons (re-registrating a callback for instance, or 
some testing related activities).

> Previously all object files that were linked into the livepatch were from the 
> output of create-diff-object. This change just includes the newly built 
> object files directly. I wonder if there are any issues or subtle differences 
> when doing this? A couple of differences off the top of my head:
> 
> 1) The object file will include _everything_ (e.g. potentially unused 
> functions) while create-diff-object generally only includes the minimum that 
> is needed.

Yes, that’s right. When a patch defines new files (and thereby new object 
files) it should do it wisely.

> 
> 2) Hooks and ignored functions/sections in the new object file would not be 
> processed at all.

Not sure I follow the point here, but hooks and functions properly defined in 
new object files will be
processed and used during link time.

I have a test hotpatch that does that.

> 
> The was previously a patch on xen-devel which took a different approach 
> (https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg03532.html). 
> Perhaps you could look at it and see which approach would be better?

Taking a look. Thanks.

> 
> Thanks,
> -- 
> Ross Lagerwall
> 
> 

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes


> On 29. Apr 2019, at 16:21, Ross Lagerwall  wrote:
> 
> On 4/29/19 3:10 PM, Ross Lagerwall wrote:
>> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>>> Hard-coding the special section group sizes is unreliable. Instead,
>>> determine them dynamically by finding the related struct definitions
>>> in the DWARF metadata.
>>> 
>>> This is a livepatch backport of kpatch upstream commit [1]:
>>> kpatch-build: detect special section group sizes 170449847136a48b19fc
>>> 
>>> Xen only deals with alt_instr, bug_frame and exception_table_entry
>>> structures, so sizes of these structers are obtained from xen-syms.
>> structers -> structures

Thanks, will fix.

>>> 
>>> This change is needed since with recent Xen the alt_instr structure
>>> has changed size from 12 to 14 bytes.
>>> 
>>> [1] 
>>> https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56
>>>  
>>> 
>>> Signed-off-by: Pawel Wieczorkiewicz 
>>> Reviewed-by: Bjoern Doebel 
>>> Reviewed-by: Martin Mazein 
>>> ---
>>>   create-diff-object.c | 60 
>>> 
>>>   livepatch-build  | 23 
>>>   2 files changed, 74 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/create-diff-object.c b/create-diff-object.c
>>> index 1e6e617..b0b4dcb 100644
>>> --- a/create-diff-object.c
>>> +++ b/create-diff-object.c
>>> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct 
>>> kpatch_elf *kelf)
>>>   }
>>>   }
>>> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 16; }
>>> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { 
>>> return 8; }
>>> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) 
>>> { return 12; }
>>> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +static int size = 0;
>>> +char *str;
>>> +if (!size) {
>>> +str = getenv("BUG_STRUCT_SIZE");
>>> +size = str ? atoi(str) : 8;
>> Why did you remove the hard error here from the original kpatch commit? I 
>> think a hard error is preferable to guessing.

Previously the sizes were hard-coded. I prefer to use them directly over 
failing hard here.
If we could not come up with a sane defaults, than failing hard would be the 
only option.
Anyway, I am cool to go either way upon your good judgment.

>>> +}
>>> +
>>> +return size;
>>> +}
>>> +
>>> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +static int size = 0;
>>> +char *str;
>>> +if (!size) {
>>> +str = getenv("BUG_STRUCT_SIZE");
>>> +size = str ? atoi(str) : 16;
>>> +}
>>> +
>>> +return size;
>>> +}
>>> +
>>> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +static int size = 0;
>>> +char *str;
>>> +if (!size) {
>>> +str = getenv("EX_STRUCT_SIZE");
>>> +size = str ? atoi(str) : 8;
>>> +}
>>> +
>>> +return size;
>>> +}
>>> +
>>> +static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +static int size = 0;
>>> +char *str;
>>> +if (!size) {
>>> +str = getenv("ALT_STRUCT_SIZE");
>>> +size = str ? atoi(str) : 12;
>>> +}
>>> +
>>> +printf("altinstr_size=%d\n", size);
>>> +return size;
>>> +}
>>>   /*
>>>* The rela groups in the .fixup section vary in size.  The beginning of 
>>> each
>>> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf 
>>> *kelf, int offset)
>>>   static struct special_section special_sections[] = {
>>>   {
>>>   .name= ".bug_frames.0",
>>> -.group_size= bug_frames_0_group_size,
>>> +.group_size= bug_frames_group_size,
>>>   },
>>>   {
>>>   .name= ".bug_frames.1",
>>> -.group_size= bug_frames_1_group_size,
>>> +.group_size= bug_frames_group_size,
>>>   },
>>>   {
>>>   .name= ".bug_frames.2",
>>> -.group_size= bug_frames_2_group_size,
>>> +.group_size= bug_frames_group_size,
>>>   },
>>>   {
>>>   .name= ".bug_frames.3",
>>> diff --git a/livepatch-build b/livepatch-build
>>> index c057fa1..a6cae12 100755
>>> --- a/livepatch-build
>>> +++ b/livepatch-build
>>> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
>>>   echo ""
>>>   echo
>>> +if [ -f "$XENSYMS" ]; then
>>> +echo "Reading special section data"
>>> +SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
>>> +   gawk --non-decimal-data '
>>> +   BEGIN { a = b = e = 0 }
>>> +

Re: [Xen-devel] [livepatch-build-tools 3/4] livepatch-build: Do not follow every symlink for patch file


> On 29. Apr 2019, at 14:40, Ross Lagerwall  wrote:
> 
> On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
>> In some build systems symlinks might be used for patch file names
>> to point from target directories to actual patches. Following those
>> symlinks breaks naming convention as the resulting built modules
>> would be named after the actual hardlink insteads of the symlink.
>> Livepatch-build obtains hotpatch name from the patch file, so it
>> should not canonicalize the file path resolving all the symlinks to
>> not lose the original symlink name.
>> Signed-off-by: Pawel Wieczorkiewicz 
>> Reviewed-by: Martin Pohlack 
>> Reviewed-by: Bjoern Doebel 
>> Reviewed-by: Norbert Manthey 
>> ---
>>  livepatch-build | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> diff --git a/livepatch-build b/livepatch-build
>> index c057fa1..796838c 100755
>> --- a/livepatch-build
>> +++ b/livepatch-build
>> @@ -265,7 +265,9 @@ done
>>  [ -z "$DEPENDS" ] && die "Build-id dependency not given"
>>SRCDIR="$(readlink -m -- "$srcarg")"
>> -PATCHFILE="$(readlink -m -- "$patcharg")"
>> +# We need an absolute path because we move around, but we need to
>> +# retain the name of the symlink (= realpath -s)
>> +PATCHFILE="$(readlink -f "$(dirname "$patcharg")")/$(basename "$patcharg")"
>>  CONFIGFILE="$(readlink -m -- "$configarg")"
>>  OUTPUT="$(readlink -m -- "$outputarg")"
>>  
> 
> This works, but would it not be simpler to just pass $patcharg into 
> make_patch_name()?
> 

No strong opinion here, but the readlink change felt less invasive (no changes 
to the existing semantics).

Thank you for looking into this.

> -- 
> Ross Lagerwall
> 


Best Regards,
Pawel Wieczorkiewicz





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section