Conversation
There was a problem hiding this comment.
10 seconds sounds like a lot to me for a message processed by systemd. Is there something else, which slows down the process?
There was a problem hiding this comment.
It is hard to pick the right value here, as the call to sd_notify() may be delayed by an arbitrary amount of time. Probably 1 second would be enough, but I chose 10 just to be on a safe side. Please note that this delay will not delay the boot process, as systemd will go ahead and start other units as soon as it gets the notification. So the only effect of a longer delay will be the shell script running for some time afterwards. On the other hand, if the notification is lost then the startup of the unit will hang for a longer time until it gets timed out by systemd and fails.
This is an ugly workaround–I believe that you can solve it properly by using MAINPID= together with sd_notify(). I will submit a bug report about it to xen.
There was a problem hiding this comment.
The sleep 10 is fragile, as pointed out by others. If it cannot be done in any other way, leave it like this, but if it can be fixed, fix it. What happened to the bug report to Xen?
There was a problem hiding this comment.
Actually, this code has since been dropped from the PR–see comment below, which I reproduce here:
Interestingly, as I just found out, the patch is not needed anymore as I cannot reproduce the problematic behaviour with systemd 233. It seems that there was a problem with systemd 232, which led to the notification being dropped. Given that, it seems that the scripts in xen are correct, and the patch can be dropped–I'll drop it from the commit.
|
Awesome! Thanks! Will test on my machines in the next couple of days.
Problems I see without testing:
* You introduce tabs in xen-dom0.nix (and I can now assume your tabstop = 8 :) )
* The systemd patch is ugly. It potentially slows down complete system
boot by 10s. Is it upstream somewhere? Can we get it from upstream?
If not, how do they solve the same problem upstream?
* Can 4.8's oxenstored build without systemd dependency? (A problem for
SLNOS which uses s6 init).
|
|
Hi!
That's an oversight–I'll fix that and amend the commit
It does not slow the boot by 10s, because systemd continues as soon as it gets the notification from oxenstored. Interestingly, as I just found out, the patch is not needed anymore as I cannot reproduce the problematic behaviour with systemd 233. It seems that there was a problem with systemd 232, which led to the notification being dropped. Given that, it seems that the scripts in xen are correct, and the patch can be dropped–I'll drop it from the commit.
Yes, oxenstored was previously built without an explicit systemd dependency. As a result, systemd was included when the |
|
I have just amended the commit removing the tabs and the race condition workaround. I have not done anything about the systemd dependency, and I am not sure how to continue. Should the packages have a |
|
As a result, systemd was included when the `xen` package was compiled, but not `xen-slim` or `xen-light`.
Eh? Why? Systemd wasn't even referenced, IIRC.
I have just amended the commit removing the tabs and the race condition workaround. I have not done anything about the systemd dependency, and I am not sure how to continue. Should the packages have a `useSystemd` flag?
Nah, don't bother, I'll do that after I test it with s6 later.
LGTM. Thanks again!
|
It seems that systemd was propagated from one of the other dependencies, and just picked up by the configure script. |
|
@grahamc can you merge this?
|
|
I would like to add security patches against this version of xen. Should I amend this PR, or create a new one? |
|
I would add security patches on top here (but as separate commits, no squashing).
|
|
I have added a commit with security patches, and rebased everything. |
There was a problem hiding this comment.
Please do not hardcode /var/log/xen. As a default it's fine.
There was a problem hiding this comment.
The code that you are referring to has been carried over from the service that was created for xen 4.5 (or earlier). An argument for keeping it is to try to minimise the change. If you insist, then I can remove it, but otherwise we may keep it as is and drop it when we drop xen 4.5.
There was a problem hiding this comment.
Looking at the rest of the module, /var/log/xen seems to be hardcoded in a lot of other places. If you want to get rid of it, then I suggest to do it with a separate PR.
There was a problem hiding this comment.
The constant "4.8" is repeated multiple times.
There was a problem hiding this comment.
Would you see a named constant here instead?
There was a problem hiding this comment.
Yes, just to make sure people don't make search and replace errors.
There was a problem hiding this comment.
If this is important, create an issue for this.
There was a problem hiding this comment.
This is also carried over from the previous package. I'm not sure if I understand this TODO. Is the idea to use ipxe packaged separately?
|
michalpalka <notifications@github.com> writes:
Is the idea to use ipxe packaged separately?
Yes, to make xen-light even lighter.
Actually, this code has since been dropped from the PR–see comment below, which I reproduce here:
No, I just fetched this branch and its still there.
Also the tabs in xen-dom0.nix are still there.
|
I'll create an issue.
It seems that I have dropped the reference to the patch, but not the patch itself–sorry for that, I will fix it.
That's right–I'll fix it too. |
This commit adds the xen_4_8 package to be used instead of xen (currently at 4.5.5): * Add packages xen_4_8, xen_4_8-slim and xen_4_8-light * Add packages qemu_xen_4_8 and qemu_xen_4_8-light to be used with xen_4_8-slim and xen_4_8-light respectively. * Add systemd to buildInputs of xen (it is required by oxenstored) * Adapt xen service to work with the new version of xen * Use xen-init-dom0 to initlilise dom0 in xen-store * Currently, the virtualisation.xen.stored option is ignored if xen 4.8 is used
…4.8) This commit contains security patches for xen 4.8. The patches for XSA-216 applied to the kernel are omitted, as they are part of 80e0cda. XSA-216 Issue Description: > The block interface response structure has some discontiguous fields. > Certain backends populate the structure fields of an otherwise > uninitialized instance of this structure on their stacks, leaking > data through the (internal or trailing) padding field. More: https://xenbits.xen.org/xsa/advisory-216.html XSA-217 Issue Description: > Domains controlling other domains are permitted to map pages owned by > the domain being controlled. If the controlling domain unmaps such a > page without flushing the TLB, and if soon after the domain being > controlled transfers this page to another PV domain (via > GNTTABOP_transfer or, indirectly, XENMEM_exchange), and that third > domain uses the page as a page table, the controlling domain will have > write access to a live page table until the applicable TLB entry is > flushed or evicted. Note that the domain being controlled is > necessarily HVM, while the controlling domain is PV. More: https://xenbits.xen.org/xsa/advisory-217.html XSA-218 Issue Description: > We have discovered two bugs in the code unmapping grant references. > > * When a grant had been mapped twice by a backend domain, and then > unmapped by two concurrent unmap calls, the frontend may be informed > that the page had no further mappings when the first call completed rather > than when the second call completed. > > * A race triggerable by an unprivileged guest could cause a grant > maptrack entry for grants to be "freed" twice. The ultimate effect of > this would be for maptrack entries for a single domain to be re-used. More: https://xenbits.xen.org/xsa/advisory-218.html XSA-219 Issue Description: > When using shadow paging, writes to guest pagetables must be trapped and > emulated, so the shadows can be suitably adjusted as well. > > When emulating the write, Xen maps the guests pagetable(s) to make the final > adjustment and leave the guest's view of its state consistent. > > However, when mapping the frame, Xen drops the page reference before > performing the write. This is a race window where the underlying frame can > change ownership. > > One possible attack scenario is for the frame to change ownership and to be > inserted into a PV guest's pagetables. At that point, the emulated write will > be an unaudited modification to the PV pagetables whose value is under guest > control. More: https://xenbits.xen.org/xsa/advisory-219.html XSA-220 Issue Description: > Memory Protection Extensions (MPX) and Protection Key (PKU) are features in > newer processors, whose state is intended to be per-thread and context > switched along with all other XSAVE state. > > Xen's vCPU context switch code would save and restore the state only > if the guest had set the relevant XSTATE enable bits. However, > surprisingly, the use of these features is not dependent (PKU) or may > not be dependent (MPX) on having the relevant XSTATE bits enabled. > > VMs which use MPX or PKU, and context switch the state manually rather > than via XSAVE, will have the state leak between vCPUs (possibly, > between vCPUs in different guests). This in turn corrupts state in > the destination vCPU, and hence may lead to weakened protections > > Experimentally, MPX appears not to make any interaction with BND* > state if BNDCFGS.EN is set but XCR0.BND{CSR,REGS} are clear. However, > the SDM is not clear in this case; therefore MPX is included in this > advisory as a precaution. More: https://xenbits.xen.org/xsa/advisory-220.html XSA-221 Issue Description: > When polling event channels, in general arbitrary port numbers can be > specified. Specifically, there is no requirement that a polled event > channel ports has ever been created. When the code was generalised > from an earlier implementation, introducing some intermediate > pointers, a check should have been made that these intermediate > pointers are non-NULL. However, that check was omitted. More: https://xenbits.xen.org/xsa/advisory-221.html XSA-222 Issue Description: > Certain actions require removing pages from a guest's P2M > (Physical-to-Machine) mapping. When large pages are in use to map > guest pages in the 2nd-stage page tables, such a removal operation may > incur a memory allocation (to replace a large mapping with individual > smaller ones). If this allocation fails, these errors are ignored by > the callers, which would then continue and (for example) free the > referenced page for reuse. This leaves the guest with a mapping to a > page it shouldn't have access to. > > The allocation involved comes from a separate pool of memory created > when the domain is created; under normal operating conditions it never > fails, but a malicious guest may be able to engineer situations where > this pool is exhausted. More: https://xenbits.xen.org/xsa/advisory-222.html XSA-224 Issue Description: > We have discovered a number of bugs in the code mapping and unmapping > grant references. > > * If a grant is mapped with both the GNTMAP_device_map and > GNTMAP_host_map flags, but unmapped only with host_map, the device_map > portion remains but the page reference counts are lowered as though it > had been removed. This bug can be leveraged cause a page's reference > counts and type counts to fall to zero while retaining writeable > mappings to the page. > > * Under some specific conditions, if a grant is mapped with both the > GNTMAP_device_map and GNTMAP_host_map flags, the operation may not > grab sufficient type counts. When the grant is then unmapped, the > type count will be erroneously reduced. This bug can be leveraged > cause a page's reference counts and type counts to fall to zero while > retaining writeable mappings to the page. > > * When a grant reference is given to an MMIO region (as opposed to a > normal guest page), if the grant is mapped with only the > GNTMAP_device_map flag set, a mapping is created at host_addr anyway. > This does *not* cause reference counts to change, but there will be no > record of this mapping, so it will not be considered when reporting > whether the grant is still in use. More: https://xenbits.xen.org/xsa/advisory-224.html
|
I have pushed new versions of commits, which hopefully address all concerns.
Actually, the reference to the patch was also there. I must have pushed old (unamended) commits by mistake. Thanks for the vigilance. |
This commit adds the xen_4_8 package to be used instead of
xen (currently at 4.5.5):
with xen_4_8-slim and xen_4_8-light respectively.
if xen 4.8 is used
Motivation for this change
This introduces Xen 4.8.1, which is the most current version, along with a few needed changes and a few fixes.
To use the new xen, use these configuration options.
xen_4_8-slimmay be used with this configuration:Finally,
xen_4_8-lightmay be used with this configuration:One notable limitation of this PR is that the
virtualisation.xen.storedis ignored with xen 4.8, which I hope to address in the future.Furthermore, there are a few statements in
xen-dom0.nixandgeneric.nixthat are conditional on the xen version, which is not ideal. Please let me know if you would like it to be handled differently.Also, the package refers to the fork https://github.com/michalpalka/xen-libhvm of the
xen-libhvmrepo, which needed to be adapted to work with xen 4.8. This might equally be solved using a patch.Please let me know if you have any feedback.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)