|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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 <julien.grall@xxxxxxx> 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 <wipawel@xxxxxxxxx>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@xxxxxxxxxx>
>> Reviewed-by: Eslam Elnikety <elnikety@xxxxxxxxx>
>> Reviewed-by: Petre Eftime <epetre@xxxxxxxxxx>
>> Reviewed-by: Leonard Foerster <foersleo@xxxxxxxxx>
>> Reviewed-by: Martin Pohlack <mpohlack@xxxxxxxxx>
>> Reviewed-by: Norbert Manthey <nmanthey@xxxxxxxxx>
>> ---
>> v2:
>> * Bump XEN_SYSCTL_INTERFACE_VERSION to 0x00000013
>> ---
>> 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
>> +++ b/tools/libxc/xc_misc.c
>> @@ -854,7 +854,8 @@ int xc_livepatch_list(xc_interface *xch, unsigned int
>> max, unsigned int start,
>> static int _xc_livepatch_action(xc_interface *xch,
>> char *name,
>> unsigned int action,
>> - uint32_t timeout)
>> + uint32_t timeout,
>> + uint64_t flags)
>> {
>> int rc;
>> DECLARE_SYSCTL;
>> @@ -880,6 +881,7 @@ static int _xc_livepatch_action(xc_interface *xch,
>> sysctl.u.livepatch.pad = 0;
>> sysctl.u.livepatch.u.action.cmd = action;
>> sysctl.u.livepatch.u.action.timeout = timeout;
>> + sysctl.u.livepatch.u.action.flags = flags;
>> sysctl.u.livepatch.u.action.name = def_name;
>> set_xen_guest_handle(sysctl.u.livepatch.u.action.name.name, name);
>> @@ -891,24 +893,24 @@ static int _xc_livepatch_action(xc_interface *xch,
>> return rc;
>> }
>> -int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout)
>> +int xc_livepatch_apply(xc_interface *xch, char *name, uint32_t timeout,
>> uint64_t flags)
>> {
>> - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_APPLY, timeout);
>> + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_APPLY, timeout,
>> flags);
>> }
>> -int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout)
>> +int xc_livepatch_revert(xc_interface *xch, char *name, uint32_t timeout,
>> uint64_t flags)
>> {
>> - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REVERT,
>> timeout);
>> + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REVERT,
>> timeout, flags);
>> }
>> -int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout)
>> +int xc_livepatch_unload(xc_interface *xch, char *name, uint32_t timeout,
>> uint64_t flags)
>> {
>> - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_UNLOAD,
>> timeout);
>> + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_UNLOAD,
>> timeout, flags);
>> }
>> -int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout)
>> +int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout,
>> uint64_t flags)
>> {
>> - return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE,
>> timeout);
>> + return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE,
>> timeout, flags);
>> }
>> /*
>> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
>> index 3233472157..a37b2457ff 100644
>> --- a/tools/misc/xen-livepatch.c
>> +++ b/tools/misc/xen-livepatch.c
>> @@ -23,18 +23,23 @@ void show_help(void)
>> {
>> fprintf(stderr,
>> "xen-livepatch: live patching tool\n"
>> - "Usage: xen-livepatch <command> [args]\n"
>> + "Usage: xen-livepatch <command> [args] [command-flags]\n"
>> " <name> An unique name of payload. Up to %d characters.\n"
>> "Commands:\n"
>> " help display this help\n"
>> " upload <name> <file> upload file <file> with <name> name\n"
>> " list list payloads uploaded.\n"
>> - " apply <name> apply <name> patch.\n"
>> + " apply <name> [flags] apply <name> patch.\n"
>> + " Supported flags:\n"
>> + " --nodeps Disable inter-module buildid
>> dependency check.\n"
>> + " Check only against hypervisor
>> buildid.\n"
>> " revert <name> revert name <name> patch.\n"
>> " replace <name> apply <name> patch and revert all
>> others.\n"
>> " unload <name> unload name <name> patch.\n"
>> - " load <file> upload and apply <file>.\n"
>> - " name is the <file> name\n",
>> + " load <file> [flags] upload and apply <file> with name as
>> the <file> name\n"
>> + " Supported flags:\n"
>> + " --nodeps Disable inter-module buildid
>> dependency check.\n"
>> + " Check only against hypervisor
>> buildid.\n",
>> XEN_LIVEPATCH_NAME_SIZE);
>> }
>> @@ -225,12 +230,13 @@ static int upload_func(int argc, char *argv[])
>> return rc;
>> }
>> -/* These MUST match to the 'action_options[]' array slots. */
>> +/* These MUST match to the 'action_options[]' and 'flag_options[]' array
>> slots. */
>> enum {
>> ACTION_APPLY = 0,
>> ACTION_REVERT = 1,
>> ACTION_UNLOAD = 2,
>> ACTION_REPLACE = 3,
>> + ACTION_NUM
>> };
>> struct {
>> @@ -238,7 +244,7 @@ struct {
>> int expected; /* The state to be in after the function. */
>> const char *name;
>> const char *verb;
>> - int (*function)(xc_interface *xch, char *name, uint32_t timeout);
>> + int (*function)(xc_interface *xch, char *name, uint32_t timeout,
>> uint64_t flags);
>> } action_options[] = {
>> { .allow = LIVEPATCH_STATE_CHECKED,
>> .expected = LIVEPATCH_STATE_APPLIED,
>> @@ -266,6 +272,66 @@ struct {
>> },
>> };
>> +/*
>> + * This structure defines supported flag options for actions.
>> + * It defines entries for each action and supports up to 64
>> + * flags per action.
>> + */
>> +struct {
>> + const char *name;
>> + const uint64_t flag;
>> +} flag_options[ACTION_NUM][8 * sizeof(uint64_t)] = {
>> + { /* ACTION_APPLY */
>> + { .name = "--nodeps",
>> + .flag = LIVEPATCH_ACTION_APPLY_NODEPS,
>> + },
>> + },
>> + { /* ACTION_REVERT */
>> + },
>> + { /* ACTION_UNLOAD */
>> + },
>> + { /* ACTION_REPLACE */
>> + }
>> +};
>> +
>> +/*
>> + * 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;
>> +}
>> +
>> /* 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, &flags) )
>> 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 <id> [flags] input requirement, which must be
>> constructed */
>> + apply_argv = (char **) malloc(argc * sizeof(*apply_argv));
>> + if ( !apply_argv )
>> + return rc;
>> +
>> /* <file> */
>> - new_argv[1] = argv[0];
>> + upload_argv[1] = argv[0];
>> /* Synthesize the <id> */
>> 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 /* <id> <file> */, new_argv);
>> + /* Fill in all user provided flags */
>> + for ( i = 0; i < argc - 1; i++ )
>> + apply_argv[i + 1] = argv[i + 1];
>> +
>> + rc = upload_func(2 /* <id> <file> */, upload_argv);
>> if ( rc )
>> - return rc;
>> + goto error;
>> - rc = action_func(1 /* only <id> */, new_argv, ACTION_APPLY);
>> + rc = action_func(argc, apply_argv, ACTION_APPLY);
>> if ( rc )
>> - action_func(1, new_argv, ACTION_UNLOAD);
>> + action_func(1 /* only <id> */, upload_argv, ACTION_UNLOAD);
>> +error:
>> + free(apply_argv);
>> free(path);
>> return rc;
>> }
>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>> index 6a4af6ce57..fb91d5095c 100644
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -1575,9 +1575,17 @@ static int livepatch_action(struct
>> xen_sysctl_livepatch_action *action)
>> break;
>> }
>> - rc = build_id_dep(data, !!list_empty(&applied_list));
>> - if ( rc )
>> - break;
>> + /*
>> + * Check if action is issued with nodeps flags to ignore module
>> + * stack dependencies.
>> + */
>> + if ( !(action->flags & LIVEPATCH_ACTION_APPLY_NODEPS) )
>> + {
>> + rc = build_id_dep(data, !!list_empty(&applied_list));
>> + if ( rc )
>> + break;
>> + }
>> +
>> data->rc = -EAGAIN;
>> rc = schedule_work(data, action->cmd, action->timeout);
>> }
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index 91c48dcae0..1b2b165a6d 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -35,7 +35,7 @@
>> #include "domctl.h"
>> #include "physdev.h"
>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000012
>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
>> /*
>> * Read console content from Xen buffer ring.
>> @@ -956,6 +956,15 @@ struct xen_sysctl_livepatch_action {
>> /* hypervisor default. */
>> /* Or upper bound of time (ns)
>> */
>> /* for operation to take. */
>> +
>> +/*
>> + * Overwrite default inter-module buildid dependency chain enforcement.
>> + * Check only if module is built for given hypervisor by comparing buildid.
>> + */
>> +#define LIVEPATCH_ACTION_APPLY_NODEPS (1 << 0)
>> + uint64_t flags; /* IN: action flags. */
>> + /* Provide additional
>> parameters */
>> + /* for an action. */
>> };
>> struct xen_sysctl_livepatch_op {
>
> --
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |