[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


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
-   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
+   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


+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 
                                                     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 
-                                                   MUST 
-                                                   Must have nr of them. */
+        XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each 
+                                                   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 
                                                     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.

Ross Lagerwall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.