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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
> The payloads' name strings can be of arbitrary size (typically small
> with an upper bound of XEN_LIVEPATCH_NAME_SIZE).
> Current implementation of the list operation interface allows to copy
> names in the XEN_LIVEPATCH_NAME_SIZE chunks regardless of its actual
> size and enforces space allocation requirements on userland tools.
>
> To unify and simplify the interface, handle the name strings of
> arbitrary size by copying them in adhering chunks to the userland.
> In order to let the userland allocate enough space for the incoming
> data add an auxiliary interface xc_livepatch_list_get_sizes() that
> provides the current number of payload entries and the total size of
> all name strings. This is achieved by extending the sysctl list
> interface with an extra fields: name_total_size.
>
> The xc_livepatch_list_get_sizes() issues the livepatch sysctl list
> operation with the nr field set to 0. In this mode the operation
> returns the number of payload entries and calculates the total sizes
> for all payloads' names.
> When the sysctl operation is issued with a non-zero nr field (for
> instance with a value obtained earlier with the prior call to the
> xc_livepatch_list_get_sizes()) the new field name_total_size provides
> the total size of actually copied data.
>
> Extend the libxc to handle the name back-to-back data transfers.
>
> The xen-livepatch tool is modified to start the list operation with a
> call to the xc_livepatch_list_get_sizes() to obtain the actual number
> of payloads as well as the necessary space for names.
> The tool now always requests the actual number of entries and leaves
> the preemption handling to the libxc routine. The libxc still returns
> 'done' and 'left' parameters with the same semantic allowing the tool
> to detect anomalies and react to them. At the moment it is expected
> that the tool receives the exact number of entires as requested.
entries

> The xen-livepatch tool has been also modified to handle the name
> back-to-back transfers correctly.
>
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@xxxxxxxxx>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@xxxxxxxxxx>
> Reviewed-by: Bjoern Doebel <doebel@xxxxxxxxx>
> Reviewed-by: Martin Pohlack <mpohlack@xxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
> Changed since v1:
>    * added corresponding documentation
>
>   docs/misc/livepatch.pandoc    |  24 +++++----
>   tools/libxc/include/xenctrl.h |  49 ++++++++++++------
>   tools/libxc/xc_misc.c         | 100 ++++++++++++++++++++++++++++---------
>   tools/misc/xen-livepatch.c    | 112 ++++++++++++++++++++++--------------------
>   xen/common/livepatch.c        |  31 +++++++++---
>   xen/include/public/sysctl.h   |  15 +++---
>   6 files changed, 219 insertions(+), 112 deletions(-)
>
> diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
> index 406fb79df8..e7bcc70f5a 100644
> --- a/docs/misc/livepatch.pandoc
> +++ b/docs/misc/livepatch.pandoc
> @@ -717,17 +717,20 @@ The caller provides:
>    * `idx` Index iterator. The index into the hypervisor's payload count. It is
>       recommended that on first invocation zero be used so that `nr` (which the
>       hypervisor will update with the remaining payload count) be provided.
> -    Also the hypervisor will provide `version` with the most current value.
> +    Also the hypervisor will provide `version` with the most current value and
> +    calculated total size for all payloads' names.
>    * `nr` The max number of entries to populate. Can be zero which will result
>       in the hypercall being a probing one and return the number of payloads
>       (and update the `version`).
>    * `pad` - *MUST* be zero.
>    * `status` Virtual address of where to write `struct xen_livepatch_status`
>      structures. Caller *MUST* allocate up to `nr` of them.
> - * `name` - Virtual address of where to write the unique name of the payload.
> -   Caller *MUST* allocate up to `nr` of them. Each *MUST* be of
> -   **XEN_LIVEPATCH_NAME_SIZE** size. Note that **XEN_LIVEPATCH_NAME_SIZE** 
> includes
> -   the NUL terminator.
> + * `name` - Virtual address of where to write the unique name of the payloads.
> +   Caller *MUST* allocate enough space to be able to store all received data
> +   (i.e. total allocated space *MUST* match the `name_total_size` value
> +   provided by the hypervisor). Individual payload name cannot be longer than
> +   **XEN_LIVEPATCH_NAME_SIZE** bytes. Note that **XEN_LIVEPATCH_NAME_SIZE**
> +   includes the NUL terminator.
>    * `len` - Virtual address of where to write the length of each unique name
>      of the payload. Caller *MUST* allocate up to `nr` of them. Each *MUST* be
>      of sizeof(uint32_t) (4 bytes).
> @@ -736,7 +739,8 @@ If the hypercall returns an positive number, it is the 
> number (upto `nr`
>   provided to the hypercall) of the payloads returned, along with `nr` updated
>   with the number of remaining payloads, `version` updated (it may be the same
>   across hypercalls - if it varies the data is stale and further calls could
> -fail). The `status`, `name`, and `len` are updated at their designed index
> +fail) and the `name_total_size` containing total size of transfered data for
transferred

> +the array. The `status`, `name`, and `len` are updated at their designed index
>   value (`idx`) with the returned value of data.
>   
>   If the hypercall returns -XEN_E2BIG the `nr` is too big and should be
> @@ -775,11 +779,13 @@ The structure is as follow:
>                                                      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 */
>           XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status;  /* OUT. Must 
> have enough
>                                                      space allocate for nr of 
> them. */
> -        XEN_GUEST_HANDLE_64(char) id;           /* OUT: Array of names. Each 
> member
> -                                                   MUST 
> XEN_LIVEPATCH_NAME_SIZE in size.
> -                                                   Must have nr of them. */
> +        XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each 
> member
> +                                                   may have an arbitrary 
> length up to
> +                                                   XEN_LIVEPATCH_NAME_SIZE 
> bytes. Must have
> +                                                   nr of them. */
>           XEN_GUEST_HANDLE_64(uint32) len;        /* OUT: Array of lengths of 
> name's.
>                                                      Must have nr of them. */
>       };
> diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
> index a37b2457ff..8ac3d567fc 100644
> --- a/tools/misc/xen-livepatch.c
> +++ b/tools/misc/xen-livepatch.c
> @@ -64,14 +64,14 @@ static const char *state2str(unsigned int state)
>       return names[state];
>   }
>   
> -/* This value was choosen adhoc. It could be 42 too. */
> -#define MAX_LEN 11
>   static int list_func(int argc, char *argv[])
>   {
> -    unsigned int idx, done, left, i;
> +    unsigned int nr, done, left, i;
>       xen_livepatch_status_t *info = NULL;
>       char *name = NULL;
>       uint32_t *len = NULL;
> +    uint64_t name_total_size;
> +    off_t name_off;
If name_total_size becomes 32-bit, then I think you can replace the few 
usages of off_t with just a uint32_t (it doesn't need to be signed).
Otherwise looks good to me.

Thanks,
--
Ross Lagerwall

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