Re: [PATCH v2 01/13] xsplice: Design document (v5).

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: wei.liu2@citrix.com, ian.campbell@citrix.com,
	andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com,
	mpohlack@amazon.com, ross.lagerwall@citrix.com,
	stefano.stabellini@citrix.com, sasha.levin@oracle.com,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 01/13] xsplice: Design document (v5).
Date: Fri, 5 Feb 2016 16:47:37 -0500	[thread overview]
Message-ID: <20160205214737.GA13693@char.us.oracle.com> (raw)
In-Reply-To: <56B4CCF402000078000CF288@prv-mh.provo.novell.com>

I've snipped the email. I've taken your reviews in account - and just
responding on some of them that I believe need more comments.

..snip..
> > +However it has severe drawbacks - the safety checks which have to make sure
> > +the function is not on the stack - must also check every caller. For some
> > +patches this could mean - if there were an sufficient large amount of
> > +callers - that we would never be able to apply the update.
> 
> While this is an issue, didn't we settle on doing the patching without
> any deep call stacks only? Also why would that problem not apply to
> to trampoline example right below this section (after all you can't
> just go and patch a multi-byte instruction without making sure no
> CPU is about to execute that code).

True. I massaged the comment and added another in the next one.

The thinking is that if we do inline patching we have way more of changes to
do as opposed to function patching. Hence the 'drawback' of doing the inline
patching is that we may not be able to satisfy the safety checks within
the time alloted. While the function has less of stacks to check.

Granted this is a bit up in the air - as we elected to do the patching on
code paths that have very defined stacks. But not sure if I should include that
in the design as opposed to the implementation.

> 
> > +As such having the payload in an ELF file is the sensible way. We would be
> > +carrying the various sets of structures (and data) in the ELF sections under
> > +different names and with definitions. The prefix for the ELF section name
> > +would always be: *.xsplice* to match up to the names of the structures.
> 
> Note that the use of * here is confusing - do you mean them to
> represent quotes (matching up with this supposedly being a prefix)
> or as wild card?

.. That had evolved a bit. Lets remove that - as earlier versions had
.xsplice for everything: .xsplice.reloc, .xsplice.data, ... etc.

> 
> > +The xSplice core code loads the payload as a standard ELF binary, relocates it
> > +and handles the architecture-specifc sections as needed. This process is much
> > +like what the Linux kernel module loader does.
> > +
> > +The payload contains a section (xsplice_patch_func) with an array of structures
> > +describing the functions to be patched:
> > +<pre>
> > +struct xsplice_patch_func {  
> > +    const char *name;  
> > +    unsigned long new_addr;  
> > +    const unsigned long old_addr;  
> 
> Stray(?, and slightly confusing) const here...
> 
> > +    uint32_t new_size;  
> > +    const uint32_t long old_size;  
> 
> ... and here. This one also leaves the reader guess about the
> actual type meant.

Ah, I put the const there in anticipation of you wanting an const!

> 
> Also is using "long" here really a good idea? Shouldn't we rather use
> fixed width or ELF types?

We can. It would look like this:

struct xsplice_patch_func {
    const unsigned char *name;
    Elf64_Xword new_addr;
    Elf64_Xword old_addr;
    Elf64_Word new_size;
    Elf64_Word old_size;
    uint8_t pad[32];
};

Much nicer.
> 
> > +* `old_size` and `new_size` contain the sizes of the respective functions in bytes.
> > +   The value **MUST** not be zero.
> 
> For old_size I can see this, but can't new_size being zero "NOP out
> the entire code sequence"?

The patchset does not (yet) support that. Nor the short branch instructions.
I am trying to keep the amount of 'features' to the minimum so that reviews
can be easier.

Let me add in todo list (v2: Not Yet Done) this request.

..snip..
> > +### XEN_SYSCTL_XSPLICE_LIST (2)
> > +
> > +Retrieve an array of abbreviated status and names of payloads that are loaded in the
> > +hypervisor.
> > +
> > +The caller provides:
> > +
> > + * `version`. Initially (on first hypercall) *MUST* be zero.
> > + * `idx` index iterator. On first call *MUST* be zero, subsequent calls varies.
> > + * `nr` the max number of entries to populate.
> > + * `pad` - *MUST* be zero.
> > + * `status` virtual address of where to write `struct xen_xsplice_status`
> > +   structures. Caller *MUST* allocate up to `nr` of them.
> > + * `id` - virtual address of where to write the unique id of the payload.
> > +   Caller *MUST* allocate up to `nr` of them. Each *MUST* be of
> > +   **XEN_XSPLICE_NAME_SIZE** size.
> > + * `len` - virtual address of where to write the length of each unique id
> > +   of the payload. Caller *MUST* allocate up to `nr` of them. Each *MUST* be
> > +   of sizeof(uint32_t) (4 bytes).
> > +
> > +If the hypercall returns an positive number, it is the number (up to `nr`)
> > +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`,
> > +`id`, and `len`' are updated at their designed index value (`idx`) with
> > +the returned value of data.
> > +
> > +If the hypercall returns E2BIG the `count` is too big and should be
> > +lowered.
> 
> s/count/nr/ ?
> 
> > +This operation can be preempted by the hypercall returning XEN_EAGAIN.
> > +Retry.
> 
> Why is this necessary when preemption via the 'nr' field is already
> possible?

I should explain that the XEN_EAGAIN is the mechanism by which the hypervisor
signals that it could only fulfill its 'nr' value.

> 
> Also what meaning would have zero as a return value here (not
> spelled out above afaics)?

Added it in - it means there is absolutly no payloads uploaded.
..snip..
> 
> > +Note that patching functions that copy to or from guest memory requires
> > +to support alternative support. This is due to SMAP (specifically *stac*
> > +and *clac* operations) which is enabled on Broadwell and later architectures.
> 
> I think it should be emphasized that this is an example, and there
> are other uses of alternative instructions (and likely more to come).
> 
> > +The v2 design must also have a mechanism for:
> > +
> > + *  An dependency mechanism for the payloads. To use that information to load:
> > +    - The appropiate payload. To verify that payload is built against the
> > +      hypervisor. This can be done via the `build-id`
> > +      or via providing an copy of the old code - so that the hypervisor can
> > +       verify it against the code in memory.
> 
> I was missing this above - do you really intend to do patching without
> at least one of those two safety measures?

Ross wrote the patches and I will make them part of the patch series. But the
problem is that there will be now over 30 patches - so to make it easier
to review I was thinking to roll them out in 'waves'. I can most certainly
include it in the next posting.
> 
> > +## Signature checking requirements.
.. snip..
> > +struct elf_payload_signature {  
> > +	u8	algo;		/* Public-key crypto algorithm PKEY_ALGO_*. */  
> > +	u8	hash;		/* Digest algorithm: HASH_ALGO_*. */  
> > +	u8	id_type;	/* Key identifier type PKEY_ID*. */  
> > +	u8	signer_len;	/* Length of signer's name */  
> > +	u8	key_id_len;	/* Length of key identifier */  
> > +	u8	__pad[3];  
> > +	__be32	sig_len;	/* Length of signature data */  
> > +};
> > +
> > +</pre>
> > +(Note that this has been borrowed from Linux module signature code.).
> 
> It doesn't make clear who's supposed to do that verification. If
> the hypervisor, this would seem to imply a whole lot of
> cryptography code needing importing...

Oh yes :-) Which is why this is on the 'Not Yet Done' part.

> 
> Jan