Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OXT-1503: Convey XSM/Flask SID sender context in Argo messages #1235

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dozylynx
Copy link
Contributor

Patches for both Xen and the Linux driver, either side of the updated ABI.

Redefines the message header format to accommodate the new 32-bit XSM/Flask SID value field that provides context from the hypervisor about the sender domain to the receiver domain, and adjusting the size of the message_type field from 32-bit to 8-bit, which also affects the range of values accepted for the message_type argument of the sendv hypercall operation. The fixed protocol indicator values for DGRAM and STREAM on the Linux side are revised to fit within the narrower 8-bit range.

Signed-off-by: Christopher Clark [email protected]

@dpsmith
Copy link
Member

dpsmith commented Aug 15, 2019

Overall looks good to me. One question that does not have to be resolved for this to go in, how long should the driver patch remain a patch vs being PR'ed to the linux-xen-argo project?

Copy link
Contributor

@jandryuk jandryuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

-#define ARGO_PROTO_DGRAM 0x6447724d
-#define ARGO_PROTO_STREAM 0x3574526d
+#define ARGO_PROTO_DGRAM 0x01
+#define ARGO_PROTO_STREAM 0x02
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't have to be addressed in this patch, but you now have ARGO_PROTO_DGRAM=1 & ARGO_PROTO_STREAM=2 here and ARGO_PTYPE_DGRAM=1 & ARGO_PTYPE_STREAM=2, in argo-linux/include/linux/argo_dev.h. Can ARGO_PROTO_DGRAM/STREAM just be converted to ARGO_PTYPE_*. It's not clear why argo_dev.h exposes ptype.

Also, looking in /usr/include/bits/socket_type.h the defines are flipped compared to argo:
SOCK_STREAM = 1
SOCK_DGRAM = 2

Copy link
Member

@dpsmith dpsmith Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To pile on, would there be a reason why we don't just drop "ARGO_PROTO_*" and use the "standard" socket types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it... so prior to this change there was a clear distinction between the magic-valued protocol indicator that went between domains (ARGO_PROTO_DGRAM/STREAM) and the protocol indicator that goes between userspace and the kernel (ARGO_PTYPE_DGRAM/STREAM),
eg. supplied in the argument to bind or returned by the ioctl.

After this PR which effectively removes the magic values, yep, it looks like using standard sock_type values for both would be fine. There's some conversion logic in libargo which converts between ARGO_PTYPE and SOCK values which could go too.

It gets slightly more painful to do the above in a patch carried within xenclient-oe because we have several argo recipes as part of resolving our exotic circular dependencies in that layer, and it would touch multiple of them. That points towards running a branch on the linux-xen-argo repository for OpenXT to do this work.

@jandryuk
Copy link
Contributor

It'd be nice to tag or branch linux-xen-argo with a xen-4.12 version in case someone on that version tries to use the driver.

@jean-edouard
Copy link
Member

@dozylynx
Copy link
Contributor Author

It'd be nice to tag or branch linux-xen-argo with a xen-4.12 version in case someone on that version tries to use the driver.

agreed; done: https://github.com/OpenXT/linux-xen-argo/releases/tag/xen-4.12

@jandryuk
Copy link
Contributor

Thanks for tagging, @dozylynx

@dpsmith
Copy link
Member

dpsmith commented Sep 13, 2019

@dozylynx would you mind resolving the conflict and confirm if you want to reject or resolve the last comments by @jandryuk and myself?

@dozylynx
Copy link
Contributor Author

@dpsmith : yes, I will respin this to resolve the conflict and address the comments by @jandryuk and yourself. I agree with the suggestion to switch to using the standard sock_type and will look into how to go about that.

@dozylynx
Copy link
Contributor Author

ok, I didn't get an immediate test pass when testing the respin of this with an incremental build, so I'll pick this up tomorrow when I have a new full build completed.

@dozylynx
Copy link
Contributor Author

(build is still under way to enable testing of the respin to resolve the conflict)
I've just filed this ticket: https://openxt.atlassian.net/browse/OXT-1691
"Argo: Use Linux sock_type values for communicating protocol"
for tracking work on addressing the comments about the protocol value with the sock_type enum.

The recent change to quiet the AVC issued when a domain does not have
XSM argo enabled unfortunately omitted a change to add initialization
logic to the dummy policy, so when running with the dummy policy active,
it results in a crash in Xen on boot.
This change adds the needed initialization.

Fixes OXT-1692.

Signed-off-by: Christopher Clark <[email protected]>
@dozylynx
Copy link
Contributor Author

I'm going to respin this PR on top of the code for this one:

https://openxt.atlassian.net/browse/OXT-1692
#1260

Convey the peer Flask label of a communicating domain by introducing
the 32-bit Flask SID in the xen_argo_ring_message_header that is
delivered with each Argo message.

To retain the existing size of the message header while adding this
32-bit field the message_type field is resized from 32 to 8 bits and
a 16-bit padding field is removed. 8-bits of padding remain.

This is an ABI change and a corresponding update to the Linux Argo
device driver is required.

Signed-off-by: Christopher Clark <[email protected]>
Updates the Linux device driver to a revised Argo ABI where the
XSM/Flask SID of the sender domain is conveyed with each message.

The ring message header has been changed to accommodate the new data and
the message_type field is reduced in size to 8-bits. Stream and datagram
protocol indicator values are redefined to within the 8-bit range.

Works in conjunction with the corresponding patch for Xen.

Signed-off-by: Christopher Clark <[email protected]>
@dozylynx dozylynx force-pushed the oxt-1503-argo-xsm-context branch from 53170f4 to 479a941 Compare September 18, 2019 18:07
@dozylynx
Copy link
Contributor Author

@dpsmith : have pushed an updated version of the commits to resolve the conflict to the branch for this PR. Observing the encounter with OXT-1692 during this work suggests that test coverage is currently too sparse, which supports looking at OXT-1683 ("Implement BATS test case to provide test coverage of Argo functions") and keeping this PR separate from the sock_type work -- does that sound ok?

@dpsmith
Copy link
Member

dpsmith commented Sep 18, 2019

@dpsmith : have pushed an updated version of the commits to resolve the conflict to the branch for this PR. Observing the encounter with OXT-1692 during this work suggests that test coverage is currently too sparse, which supports looking at OXT-1683 ("Implement BATS test case to provide test coverage of Argo functions") and keeping this PR separate from the sock_type work -- does that sound ok?

That is fine.

@jean-edouard
Copy link
Member

@dozylynx
Copy link
Contributor Author

dozylynx commented Oct 2, 2019

In testing this PR, please note that it involves an interface change between the guest and the hypervisor, so this is relevant for upgrade: VMs with access to Argo will need to be upgraded at the same time as the hypervisor is changed.

@dozylynx
Copy link
Contributor Author

Am looking into what is involved to enable BATS to test this functionality, which means making the peer SID context reachable from userspace so that a BATS test can read it in order to verify it.

@dozylynx
Copy link
Contributor Author

Testing is blocked on a design question -- comment at: October 11, 2019, 2:03 PM Pacific
https://openxt.atlassian.net/projects/OXT/issues/OXT-1473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants