Allow setting up shared memory areas between VMs from xl config files

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file". See:

https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Then plan is to use XENMEM_add_to_physmap_batch to map the shared pages from one domU to another and use XENMEM_remove_from_physmap to cancel the sharing. A wrapper to XENMEM_add_to_physmap_batch was added in the following commit:

commit 20e725e9364cff4a29945f66986ecd88cca8743d

Now add the wrapper to XENMEM_remove_from_physmap.

Signed-off-by Zhongze Liu <[email protected]>

Reviewed-by Stefano Stabellini <[email protected]>

Acked-by Wei Liu <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Wei Liu <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
 tools/libxc/include/xenctrl.h |  4 ++++
 tools/libxc/xc_domain.c       | 11 +++++++++++
 2 files changed, 15 insertions(+)

tools/libxc/include/xenctrl.h +0 -0

index 666db0b919..0d8364ea4b 100644

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 521

Quoted 11 lines

Quoted 1 line

Quoted 1 line · On 19.10.17 at 04:36, <[email protected]> wrote:

xen/include/xsm/dummy.h +2 -1

@@ -516,7 +516,8 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
  static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
  {
      XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
  }

When all three domains are different, how does the changed policy reflect the original "d has privilege over t" requirement? I understand you want to relax the current condition, but this shouldn't come at the price of granting access when access should be denied. Nor the inverse - the current domain not having privilege over both does also not mean d doesn't have the necessary privilege over t.

I continue to think that you can't validly retrofit the new intended functionality onto the existing hypercall, even if nothing except the permission check needs to be different.

Jan

Quoted 1 line

Quoted 1 line · On 19.10.17 at 19:36, <[email protected]> wrote:

Quoted 4 lines · Jan Beulich

xen/include/xsm/dummy.h static XSM_INLINE int

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 19.10.17 at 04:36, <[email protected]> wrote:

xen/include/xsm/dummy.h +0 -0

@@ -516,7 +516,8 @@ static XSM_INLINE int 

xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 1 line

xen/include/xsm/dummy.h static XSM_INLINE int · line 516

Quoted 1 line

xen/include/xsm/dummy.h +0 -0

  static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 

*d, struct domain *t)

Quoted 15 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 521

Quoted 6 lines

xen/include/xsm/dummy.h +2 -1

  {
      XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
  }

When all three domains are different, how does the changed policy reflect the original "d has privilege over t" requirement? I understand you want to relax the current condition, but this shouldn't come at the price of granting access when access should be denied. Nor the inverse - the current domain not having privilege over both does also not mean d doesn't have the necessary privilege over t.

Quoted 5 lines

I continue to think that you can't validly retrofit the new intended functionality onto the existing hypercall, even if nothing except the permission check needs to be different.

Jan

If this operation is going to be allowed at all (and I agree it has valid use cases), then there won't be a privilege relationship between
(d) and (t) to check - they'll both be (somewhat related) domUs as far as Xen can tell. If this hypercall isn't used, adding a new hypercall (subop) is the only way I'd see to do it - and that seems very redundant as it'd need to do all the same checks except for the one about the relationship between (d) and (t). I don't see the reason why the existing hypercall should deny being used for that purpose once it's possible using other means.

The only possible problem that springs to mind is a restricted kernel interface (such as the one used by QEMU in dom0 that restricts to a single target domain) that now doesn't realize it's relaying an operation that also requires permission over (t) after only checking that the origin is allowed to modify (d).

Quoted 30 lines

Quoted 1 line

Quoted 1 line · On 19.10.17 at 19:36, <[email protected]> wrote:

Quoted 4 lines · Jan Beulich

xen/include/xsm/dummy.h static XSM_INLINE int

Quoted 4 lines

Quoted 1 line

Quoted 1 line · On 19.10.17 at 04:36, <[email protected]> wrote:

xen/include/xsm/dummy.h +0 -0

@@ -516,7 +516,8 @@ static XSM_INLINE int

xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 1 line

xen/include/xsm/dummy.h static XSM_INLINE int · line 516

Quoted 1 line

xen/include/xsm/dummy.h +0 -0

   static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain

*d, struct domain *t)

Quoted 15 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 521

Quoted 6 lines

xen/include/xsm/dummy.h +2 -1

   {
       XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
   }

When all three domains are different, how does the changed policy reflect the original "d has privilege over t" requirement? I understand you want to relax the current condition, but this shouldn't come at the price of granting access when access should be denied. Nor the inverse - the current domain not having privilege over both does also not mean d doesn't have the necessary privilege over t.

Quoted 5 lines

I continue to think that you can't validly retrofit the new intended functionality onto the existing hypercall, even if nothing except the permission check needs to be different.

Jan

Quoted 9 lines

If this operation is going to be allowed at all (and I agree it has valid use cases), then there won't be a privilege relationship between
(d) and (t) to check - they'll both be (somewhat related) domUs as far as Xen can tell. If this hypercall isn't used, adding a new hypercall (subop) is the only way I'd see to do it - and that seems very redundant as it'd need to do all the same checks except for the one about the relationship between (d) and (t). I don't see the reason why the existing hypercall should deny being used for that purpose once it's possible using other means.

One problem is, as you mention here, ...

Quoted 5 lines

The only possible problem that springs to mind is a restricted kernel interface (such as the one used by QEMU in dom0 that restricts to a single target domain) that now doesn't realize it's relaying an operation that also requires permission over (t) after only checking that the origin is allowed to modify (d).

... the delegation of privilege checking responsibility to a possibly untrusted environment. Plus, as explained before, current callers expect privilege of d over t to be validated, which isn't happening anymore with the proposed change. If the existing sub-op was to be modified, I think we'd need (with c representing the current domain)
- (d over t) || ((c over d) && (c over t)) for not regressing the pre-existing use case,
- only (c over d) && (c over t) for not permitting something that isn't intended to be permitted in the new use case.
Unless the sub-op has room for adding a flag to indicate which of the two is meant (I didn't check), I don't see a way around adding another sub-op, no matter how similar this would end up being.

Jan

Quoted 76 lines

Quoted 41 lines

Quoted 2 lines

Quoted 2 lines · On 19.10.17 at 19:36, <[email protected]> wrote:

Quoted 6 lines · Jan Beulich

xen/include/xsm/dummy.h static XSM_INLINE int

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines · On 19.10.17 at 04:36, <[email protected]> wrote:

xen/include/xsm/dummy.h +0 -0

@@ -516,7 +516,8 @@ static XSM_INLINE int

xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 3 lines

Quoted 3 lines

xen/include/xsm/dummy.h +0 -0

   static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct

domain

*d, struct domain *t)

Quoted 17 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 521

Quoted 7 lines

   {
       XSM_ASSERT_ACTION(XSM_TARGET);

xen/include/xsm/dummy.h +2 -1

-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
   }

When all three domains are different, how does the changed policy reflect the original "d has privilege over t" requirement? I understand you want to relax the current condition, but this shouldn't come at the price of granting access when access should be denied. Nor the inverse - the current domain not having privilege over both does also not mean d doesn't have the necessary privilege over t.

Quoted 5 lines

I continue to think that you can't validly retrofit the new intended functionality onto the existing hypercall, even if nothing except the permission check needs to be different.

Jan

Quoted 9 lines

If this operation is going to be allowed at all (and I agree it has valid use cases), then there won't be a privilege relationship between
(d) and (t) to check - they'll both be (somewhat related) domUs as far as Xen can tell. If this hypercall isn't used, adding a new hypercall (subop) is the only way I'd see to do it - and that seems very redundant as it'd need to do all the same checks except for the one about the relationship between (d) and (t). I don't see the reason why the existing hypercall should deny being used for that purpose once it's possible using other means.

One problem is, as you mention here, ...

Quoted 5 lines

The only possible problem that springs to mind is a restricted kernel interface (such as the one used by QEMU in dom0 that restricts to a single target domain) that now doesn't realize it's relaying an operation that also requires permission over (t) after only checking that the origin is allowed to modify (d).

... the delegation of privilege checking responsibility to a possibly untrusted environment. Plus, as explained before, current callers expect privilege of d over t to be validated, which isn't happening anymore with the proposed change. If the existing sub-op was to be modified, I think we'd need (with c representing the current domain)
- (d over t) || ((c over d) && (c over t)) for not regressing the pre-existing use case,
- only (c over d) && (c over t) for not permitting something that isn't intended to be permitted in the new use case.
Unless the sub-op has room for adding a flag to indicate which of the two is meant (I didn't check), I don't see a way around adding another sub-op, no matter how similar this would end up being.

Quoted 1 line

Jan

I would say the current lack of a check for (c over t) is an oversight, which mostly doesn't matter because the ability to modify arbitrary memory in your target is transitive in almost any security model (c can modify d's code to modify t, so a malicious c can compromise t anyway). If the three domains are all different, the only way this can happen in non-XSM is for (c) to be dom0 or for your device model to have a device model (which I don't think is forbidden, but doubt anyone uses).

I now agree that this deserves a new subop, since this code is reached via the stable memory_op and not just a domctl.

Quoted 1 line

Quoted 1 line · On 22.10.17 at 13:21, <[email protected]> wrote:

How about changing the policy to (c over d) && ((d over t) || (c over t))? Given that (c over d) is a must, which is always checked somewhere higher in the call stack as Daniel pointed out, permitting (d over t) or (c over t) actually infers
permitting the other.

- if you permit (d over t) but not (c over t):
Given (c over t),
(c) can first map the src page from (t) into its own memory space and then map this page from its own memory space to (d)'s memory space.

- if you permit (c over t) but not (d over t):
Given (d over t),
(c) can first map (d)'s pages into its own memory space and modify (d)'s code to issues a hypercall that maps (t)'s memory pages into (d)'s memory space.

Quoted 7 lines

Quoted 1 line

Quoted 1 line · On 22.10.17 at 13:21, <[email protected]> wrote:

How about changing the policy to (c over d) && ((d over t) || (c over t))? Given that (c over d) is a must, which is always checked somewhere higher in the call stack as Daniel pointed out, permitting (d over t) or (c over t) actually infers
permitting the other.

Quoted 4 lines

- if you permit (d over t) but not (c over t):
Given (c over t),
(c) can first map the src page from (t) into its own memory space and then map this page from its own memory space to (d)'s memory space.

Would that work? The page, when in (c)'s space, is still owned by (t), so I don't see how mapping into (d)'s space could become possible just because it's mapped into (c)'s.

and create a corresponding entry xsm_map_gmfn_foreign_share to the xsm structure, which will be filled with
the proposed policy.

Does this look good to you?

Cheers,

Zhongze Liu

The original dummy xsm_map_gmfn_foregin checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, which restricts some useful yet not dangerous use cases of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current domain has the proper privileges on (d) and (t), grant the access.

For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to denote if two domains can share memory through map_gmfn_foregin. 2) Change to hook to grant the access IFF the current domain has proper MMU privileges on (d) and (t), and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default xen.te to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event channels.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Daniel De Graaf <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Wei Liu <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* Change several if statements to the GCC '... = a ?: b' extention.
* lookup the current domain in the hooks instead of passing it as an arg

---
  tools/flask/policy/modules/xen.if   | 2 ++
  xen/include/xsm/dummy.h             | 3 ++-
  xen/xsm/flask/hooks.c               | 4 +++-
  xen/xsm/flask/policy/access_vectors | 4 ++++
  4 files changed, 11 insertions(+), 2 deletions(-)

tools/flask/policy/modules/xen.if +2 -0

index 55437496f6..3ffd1c6239 100644
@@ -127,6 +127,8 @@ define(`domain_comms', `
  	domain_event_comms($1, $2)
  	allow $1 $2:grant { map_read map_write copy unmap };
  	allow $2 $1:grant { map_read map_write copy unmap };
+	allow $1 $2:mmu share_mem;
+	allow $2 $1:mmu share_mem;
  ')
 
  # domain_self_comms(domain)

xen/include/xsm/dummy.h +0 -0

index b2cd56cdc5..65e7060ad5 100644

xen/include/xsm/dummy.h +0 -0

  static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)

xen/xsm/flask/hooks.c +0 -0

index f01b4cfaaa..16103bafc9 100644

Quoted 69 lines

The original dummy xsm_map_gmfn_foregin checks if source domain has the proper
privileges over the target domain. Under this policy, it's not allowed if a Dom0
wants to map pages from one DomU to another, which restricts some useful yet not
dangerous use cases of the API, such as sharing pages among DomU's by calling
XENMEM_add_to_physmap from Dom0.

For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current domain
has the proper privileges on (d) and (t), grant the access.

For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to denote if
two domains can share memory through map_gmfn_foregin. 2) Change to hook to
grant the access IFF the current domain has proper MMU privileges on (d) and (t),
and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default xen.te
to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event channels.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Daniel De Graaf <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Wei Liu <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* Change several if statements to the GCC '... = a ?: b' extention.
* lookup the current domain in the hooks instead of passing it as an
arg

---
  tools/flask/policy/modules/xen.if   | 2 ++
  xen/include/xsm/dummy.h             | 3 ++-
  xen/xsm/flask/hooks.c               | 4 +++-
  xen/xsm/flask/policy/access_vectors | 4 ++++
  4 files changed, 11 insertions(+), 2 deletions(-)

tools/flask/policy/modules/xen.if +2 -0

index 55437496f6..3ffd1c6239 100644
@@ -127,6 +127,8 @@ define(`domain_comms', `
        domain_event_comms($1, $2)
        allow $1 $2:grant { map_read map_write copy unmap };
        allow $2 $1:grant { map_read map_write copy unmap };
+       allow $1 $2:mmu share_mem;
+       allow $2 $1:mmu share_mem;
  ')
    # domain_self_comms(domain)

xen/include/xsm/dummy.h +0 -0

index b2cd56cdc5..65e7060ad5 100644

xen/include/xsm/dummy.h static XSM_INLINE int · line 523

Quoted 9 lines

xen/include/xsm/dummy.h +2 -1

@@ -516,7 +516,8 @@ static XSM_INLINE int
 xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
  static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
 *d, struct domain *t)
  {
      XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
  }

Same comment as below, the check between (current->domain) and (d) should be redundant with one higher up in the call stack. The check between (current->domain) and (t) should remain, although this *does* result in a relaxing of the existing permission checks on the call as Jan noted.

Quoted 13 lines

static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
unsigned long op)

xen/xsm/flask/hooks.c +1 -1

index f01b4cfaaa..16103bafc9 100644
@@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
 *d1, struct domain *d2)
    static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
  {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ |
 MMU__MAP_WRITE);
+    return domain_has_perm(current->domain, d, SECCLASS_MMU,

Quoted 5 lines

MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
MMU__MAP_WRITE) ?:
+ domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
}

This is at least partially redundant with the higher-level permission checks needed to get to the xenmem_add_* functions (xatp_permission_check call in xen/common/memory.c, for example). That check already verifies the permission
for (current->domain) to modify (d)'s page tables.

The other two checks here look correct.

Hi Daniel,

2017-10-20 1:36 GMT+08:00 Daniel De Graaf <[email protected]>:

Quoted 112 lines · Zhongze Liu

Quoted 69 lines

The original dummy xsm_map_gmfn_foregin checks if source domain has the proper
privileges over the target domain. Under this policy, it's not allowed if a Dom0
wants to map pages from one DomU to another, which restricts some useful yet not
dangerous use cases of the API, such as sharing pages among DomU's by calling
XENMEM_add_to_physmap from Dom0.

For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current domain
has the proper privileges on (d) and (t), grant the access.

For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to denote if
two domains can share memory through map_gmfn_foregin. 2) Change to hook to
grant the access IFF the current domain has proper MMU privileges on (d) and (t),
and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default xen.te
to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event channels.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Daniel De Graaf <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Wei Liu <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* Change several if statements to the GCC '... = a ?: b' extention.
* lookup the current domain in the hooks instead of passing it as an
arg

---
  tools/flask/policy/modules/xen.if   | 2 ++
  xen/include/xsm/dummy.h             | 3 ++-
  xen/xsm/flask/hooks.c               | 4 +++-
  xen/xsm/flask/policy/access_vectors | 4 ++++
  4 files changed, 11 insertions(+), 2 deletions(-)

tools/flask/policy/modules/xen.if +2 -0

index 55437496f6..3ffd1c6239 100644
@@ -127,6 +127,8 @@ define(`domain_comms', `
        domain_event_comms($1, $2)
        allow $1 $2:grant { map_read map_write copy unmap };
        allow $2 $1:grant { map_read map_write copy unmap };
+       allow $1 $2:mmu share_mem;
+       allow $2 $1:mmu share_mem;
  ')
    # domain_self_comms(domain)

xen/include/xsm/dummy.h +0 -0

index b2cd56cdc5..65e7060ad5 100644

xen/include/xsm/dummy.h static XSM_INLINE int · line 523

Quoted 9 lines

xen/include/xsm/dummy.h +2 -1

@@ -516,7 +516,8 @@ static XSM_INLINE int
 xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
  static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
 *d, struct domain *t)
  {
      XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
  }

Same comment as below, the check between (current->domain) and (d) should be redundant with one higher up in the call stack. The check between (current->domain) and (t) should remain, although this *does* result in a relaxing of the existing permission checks on the call as Jan noted.

Quoted 13 lines

static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
unsigned long op)

xen/xsm/flask/hooks.c +1 -1

index f01b4cfaaa..16103bafc9 100644
@@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
 *d1, struct domain *d2)
    static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
  {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ |
 MMU__MAP_WRITE);
+    return domain_has_perm(current->domain, d, SECCLASS_MMU,

Quoted 5 lines

MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
MMU__MAP_WRITE) ?:
+ domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
}

This is at least partially redundant with the higher-level permission checks needed to get to the xenmem_add_* functions (xatp_permission_check call in xen/common/memory.c, for example). That check already verifies the permission
for (current->domain) to modify (d)'s page tables.

Quoted 1 line

The other two checks here look correct.

Do you mean that the checks that verify the permission for (current->domain) to modify (d)'s page tables have already been done somewhere higher up in the call stack so that I can eliminate them in both hooks?

2017-10-20 8:34 GMT+08:00 Zhongze Liu <[email protected]>:

Quoted 115 lines

Hi Daniel,

2017-10-20 1:36 GMT+08:00 Daniel De Graaf <[email protected]>:

Quoted 112 lines · Zhongze Liu

Quoted 69 lines

The original dummy xsm_map_gmfn_foregin checks if source domain has the proper
privileges over the target domain. Under this policy, it's not allowed if a Dom0
wants to map pages from one DomU to another, which restricts some useful yet not
dangerous use cases of the API, such as sharing pages among DomU's by calling
XENMEM_add_to_physmap from Dom0.

For the dummy xsm_map_gmfn_foregin, change to policy to: IFF the current domain
has the proper privileges on (d) and (t), grant the access.

For the flask side: 1) Introduce a new av permission MMU__SHARE_MEM to denote if
two domains can share memory through map_gmfn_foregin. 2) Change to hook to
grant the access IFF the current domain has proper MMU privileges on (d) and (t),
and MMU__SHARE_MEM is allowed between (d) and (t). 3) Modify the default xen.te
to allow MMU__SHARE_MEM for normal domains that allow grant mapping/event channels.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Daniel De Graaf <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Wei Liu <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* Change several if statements to the GCC '... = a ?: b' extention.
* lookup the current domain in the hooks instead of passing it as an
arg

---
   tools/flask/policy/modules/xen.if   | 2 ++
   xen/include/xsm/dummy.h             | 3 ++-
   xen/xsm/flask/hooks.c               | 4 +++-
   xen/xsm/flask/policy/access_vectors | 4 ++++
   4 files changed, 11 insertions(+), 2 deletions(-)

tools/flask/policy/modules/xen.if +2 -0

index 55437496f6..3ffd1c6239 100644
@@ -127,6 +127,8 @@ define(`domain_comms', `
         domain_event_comms($1, $2)
         allow $1 $2:grant { map_read map_write copy unmap };
         allow $2 $1:grant { map_read map_write copy unmap };
+       allow $1 $2:mmu share_mem;
+       allow $2 $1:mmu share_mem;
   ')
     # domain_self_comms(domain)

xen/include/xsm/dummy.h +0 -0

index b2cd56cdc5..65e7060ad5 100644

xen/include/xsm/dummy.h static XSM_INLINE int · line 523

Quoted 9 lines

xen/include/xsm/dummy.h +2 -1

@@ -516,7 +516,8 @@ static XSM_INLINE int
 xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
   static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain
 *d, struct domain *t)
   {
       XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, current->domain, d) ?:
+        xsm_default_action(action, current->domain, t);
   }

Same comment as below, the check between (current->domain) and (d) should be redundant with one higher up in the call stack. The check between (current->domain) and (t) should remain, although this *does* result in a relaxing of the existing permission checks on the call as Jan noted.

Quoted 16 lines

static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d,
unsigned long op)

xen/xsm/flask/hooks.c +1 -1

index f01b4cfaaa..16103bafc9 100644
@@ -1199,7 +1199,9 @@ static int flask_remove_from_physmap(struct domain
 *d1, struct domain *d2)
     static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
   {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ |
 MMU__MAP_WRITE);
+    return domain_has_perm(current->domain, d, SECCLASS_MMU,

MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(current->domain, t, SECCLASS_MMU, MMU__MAP_READ |
MMU__MAP_WRITE) ?:

Quoted 2 lines

+        domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
   }

This is at least partially redundant with the higher-level permission checks needed to get to the xenmem_add_* functions (xatp_permission_check call in xen/common/memory.c, for example). That check already verifies the permission
for (current->domain) to modify (d)'s page tables.

Quoted 5 lines

Quoted 1 line

The other two checks here look correct.

Do you mean that the checks that verify the permission for (current->domain) to modify (d)'s page tables have already been done somewhere higher up in the call stack so that I can eliminate them in both hooks?

Although xatp_permission_chec() does check (current->domain)'s permission over (d), I'm not sure if this is the case for all the call paths that would finally lead to map_gmfn_foregin().
If the answer is yes, I would happily remove the redundant checks.

Cheers,

Zhongze Liu.

Add a new structure to the IDL familiy to represent static shared memory regions as proposed in the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Reviewed-by Stefano Stabellini <[email protected]>

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping process involves the follwing steps:

* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain, no master domain etc.
* Write infomation of static shared memory areas into the appropriate xenstore paths.
* Use xc_domain_add_to_physmap_batch to do the page sharing.
* Set the refcount of the shared region accordingly

Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on two domU's is currently not allowd on x86 (see the comments in x86/mm/p2m.c:p2m_add_foregin for more details).

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Cc Wei Liu <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* unmap the successfully mapped pages whenever rc != 0

---
 tools/libxl/Makefile         |   2 +-
 tools/libxl/libxl_arch.h     |   6 +
 tools/libxl/libxl_arm.c      |  15 ++
 tools/libxl/libxl_create.c   |  27 +++
 tools/libxl/libxl_internal.h |  13 ++
 tools/libxl/libxl_sshm.c     | 395 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_x86.c      |  18 ++
 7 files changed, 475 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_sshm.c

tools/libxl/Makefile +0 -0

index 5a861f72cb..91bc70cda2 100644

tools/libxl/Makefile +24 -0

+
+/* check if the sshm slave configs in @sshm overlap */
+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    if (!len) return 0;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
+            slave_sshms[num_slaves++] = sshms + i;
+    }
+    qsort(slave_sshms, num_slaves, sizeof(slave_sshms[0]), sshm_range_cmp);
+
+    for (i = 0; i < num_slaves - 1; ++i) {
+        if (slave_sshms[i+1]->begin < slave_sshms[i]->end) {
+            SSHM_ERROR(domid, slave_sshms[i+1]->id, "slave ranges overlap.");
+            return ERROR_INVAL;

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end */

tools/libxl/Makefile +75 -0

+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm, libxl_static_shm *msshm,
+                              xen_pfn_t *mapped, unsigned int *nmapped)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, num_success, offset;
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (msshm->end - msshm->begin) >> XC_PAGE_SHIFT;
+    num_spages = (sshm->end - sshm->begin) >> XC_PAGE_SHIFT;
+    offset = sshm->offset >> XC_PAGE_SHIFT;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (msshm->begin >> XC_PAGE_SHIFT) + offset + i;
+        gpfns[i]= (sshm->begin >> XC_PAGE_SHIFT) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    num_success = 0;
+    for (i = 0; i < num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
+                       gpfns[i] << XC_PAGE_SHIFT);
+            rc = ERROR_FAIL;
+        } else {
+            mapped[num_success++] = gpfns[i];
+        }
+    }
+    *nmapped = num_success;
+    if (rc) goto out;
+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int libxl__sshm_incref(libxl__gc *gc, xs_transaction_t xt,
+                              const char *sshm_path)
+{
+    int rc, count;
+    const char *count_path, *count_string;
+
+    count_path = GCSPRINTF("%s/users", sshm_path);
+    rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
+    if (rc) goto out;
+    count = atoi(count_string);
+
+    count_string = GCSPRINTF("%d", count+1);
+    rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
+    if (rc) goto out;
+
+    rc = 0;
+out:
+    return rc;
+}
+

tools/libxl/Makefile +21 -0

+
+        /* all checks passed, do the job */
+        if (!isretry) {
+            rc = libxl__sshm_do_map(gc, master_domid, domid,
+                                    sshm, &master_sshm,
+                                    mapped, &nmapped);
+            if (rc) goto out;
+        }
+
+        /* write the result to xenstore and commit */
+        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
+        if (rc) goto out;
+        rc = libxl__xs_writev(gc, xt, slave_path, ents);
+        if (rc) goto out;
+        rc = libxl__sshm_incref(gc, xt, sshm_path);
+        if (rc) goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+        isretry = true;

tools/libxl/Makefile +32 -0

+    }
+
+    libxl__xs_transaction_abort(gc, &xt);
+
+    return rc;
+}
+
+static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+    const char *sshm_path, *dom_path, *dom_role_path;
+    char *ents[13];
+    struct xs_permissions noperm;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = SSHM_PATH(sshm->id);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
+
+    /* prepare the xenstore entries */
+    ents[0] = "master";
+    ents[1] = GCSPRINTF("%"PRIu32, domid);
+    ents[2] = "begin";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[4] = "end";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = "cache_policy";

tools/libxl/Makefile +34 -0

+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            /* every ID can appear in each domain at most once */
+            if (libxl__xs_read(gc, xt, dom_role_path)) {
+                SSHM_ERROR(domid, sshm->id,
+                           "domain tried to map the same ID twice.");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+            rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
+            if (rc) goto out;;
+
+            libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
+            libxl__xs_writev(gc, xt, sshm_path, ents);
+        } else {
+            SSHM_ERROR(domid, sshm->id, "can only have one master.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+

tools/libxl/libxl_x86.c +13 -0

+     return false;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+    if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C

Signature · 1 line

2.14.2

Quoted 22 lines

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping process involves the follwing steps:

* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain, no master domain etc.
* Write infomation of static shared memory areas into the appropriate xenstore paths.
* Use xc_domain_add_to_physmap_batch to do the page sharing.
* Set the refcount of the shared region accordingly

Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on two domU's is currently not allowd on x86 (see the comments in x86/mm/p2m.c:p2m_add_foregin for more details).

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Quoted 8 lines

Cc Wei Liu <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

---
V3:
* unmap the successfully mapped pages whenever rc != 0

A general note: please properly capitalise the comments.

Quoted 15 lines

---
 tools/libxl/Makefile         |   2 +-
 tools/libxl/libxl_arch.h     |   6 +
 tools/libxl/libxl_arm.c      |  15 ++
 tools/libxl/libxl_create.c   |  27 +++
 tools/libxl/libxl_internal.h |  13 ++
 tools/libxl/libxl_sshm.c     | 395 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_x86.c      |  18 ++
 7 files changed, 475 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_sshm.c

tools/libxl/Makefile +0 -0

index 5a861f72cb..91bc70cda2 100644

[...]

Quoted 24 lines

tools/libxl/Makefile +24 -0

+
+/* check if the sshm slave configs in @sshm overlap */
+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    if (!len) return 0;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
+            slave_sshms[num_slaves++] = sshms + i;
+    }
+    qsort(slave_sshms, num_slaves, sizeof(slave_sshms[0]), sshm_range_cmp);
+
+    for (i = 0; i < num_slaves - 1; ++i) {
+        if (slave_sshms[i+1]->begin < slave_sshms[i]->end) {
+            SSHM_ERROR(domid, slave_sshms[i+1]->id, "slave ranges overlap.");
+            return ERROR_INVAL;

tools/libxl/Makefile

Quoted 10 lines

tools/libxl/Makefile +10 -0

+        }
+    }
+
+    return 0;
+}
+
+/*   libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ *   This functions maps
+ *     mater gfn: [@msshm->begin + @sshm->offset, @msshm->end + @sshm->offset)

"master"

This is confusing. What if offset is not page aligned?

Quoted 83 lines

+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end */

tools/libxl/Makefile +75 -0

+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm, libxl_static_shm *msshm,
+                              xen_pfn_t *mapped, unsigned int *nmapped)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, num_success, offset;
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (msshm->end - msshm->begin) >> XC_PAGE_SHIFT;
+    num_spages = (sshm->end - sshm->begin) >> XC_PAGE_SHIFT;
+    offset = sshm->offset >> XC_PAGE_SHIFT;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (msshm->begin >> XC_PAGE_SHIFT) + offset + i;
+        gpfns[i]= (sshm->begin >> XC_PAGE_SHIFT) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    num_success = 0;
+    for (i = 0; i < num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
+                       gpfns[i] << XC_PAGE_SHIFT);
+            rc = ERROR_FAIL;
+        } else {
+            mapped[num_success++] = gpfns[i];
+        }
+    }
+    *nmapped = num_success;
+    if (rc) goto out;
+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int libxl__sshm_incref(libxl__gc *gc, xs_transaction_t xt,
+                              const char *sshm_path)
+{
+    int rc, count;
+    const char *count_path, *count_string;
+
+    count_path = GCSPRINTF("%s/users", sshm_path);
+    rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
+    if (rc) goto out;
+    count = atoi(count_string);
+
+    count_string = GCSPRINTF("%d", count+1);
+    rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
+    if (rc) goto out;
+
+    rc = 0;
+out:
+    return rc;
+}
+

[...]

Quoted 21 lines

tools/libxl/Makefile +21 -0

+
+        /* all checks passed, do the job */
+        if (!isretry) {
+            rc = libxl__sshm_do_map(gc, master_domid, domid,
+                                    sshm, &master_sshm,
+                                    mapped, &nmapped);
+            if (rc) goto out;
+        }
+
+        /* write the result to xenstore and commit */
+        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
+        if (rc) goto out;
+        rc = libxl__xs_writev(gc, xt, slave_path, ents);
+        if (rc) goto out;
+        rc = libxl__sshm_incref(gc, xt, sshm_path);
+        if (rc) goto out;
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+        isretry = true;

tools/libxl/Makefile

Quoted 10 lines

tools/libxl/Makefile +10 -0

+    }
+
+    rc = 0;
+out:
+    if (rc) {
+        /* role back successfully mapped pages */
+        SSHM_ERROR(domid, sshm->id, "failed to map some pages, cancelling.");
+        for (i = 0; i < nmapped; i++) {
+            xc_domain_remove_from_physmap(CTX->xch, domid, mapped[i]);
+        }

It is possible to get here when the first attempt of mapping has failed. The do_map function can return early before it modifies nmapped. At first glance this code snippet is buggy.

I would suggest you change isretry to mapped and adjust the predicate for rolling back.

Quoted 33 lines

tools/libxl/Makefile +32 -0

+    }
+
+    libxl__xs_transaction_abort(gc, &xt);
+
+    return rc;
+}
+
+static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+    const char *sshm_path, *dom_path, *dom_role_path;
+    char *ents[13];
+    struct xs_permissions noperm;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = SSHM_PATH(sshm->id);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
+
+    /* prepare the xenstore entries */
+    ents[0] = "master";
+    ents[1] = GCSPRINTF("%"PRIu32, domid);
+    ents[2] = "begin";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[4] = "end";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = "cache_policy";

tools/libxl/Makefile

Quoted 10 lines

tools/libxl/Makefile +10 -0

+    ents[9] = libxl__strdup(gc,
+                            libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
+    ents[10] = "users";
+    ents[11] = "1";
+    ents[12] = NULL;
+
+    /* could only be accessed by Dom0 */
+    noperm.id = 0;
+    noperm.perms = XS_PERM_NONE;
+

The owner of the path is going to be Dom0 and all other domains have no permission, which matches the comment.

Do you not want domid to be able to read? If you don't want it to read from the path, why not put in under some other paths like /libxl/?

Quoted 34 lines

tools/libxl/Makefile +34 -0

+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            /* every ID can appear in each domain at most once */
+            if (libxl__xs_read(gc, xt, dom_role_path)) {
+                SSHM_ERROR(domid, sshm->id,
+                           "domain tried to map the same ID twice.");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+            rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
+            if (rc) goto out;;
+
+            libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
+            libxl__xs_writev(gc, xt, sshm_path, ents);
+        } else {
+            SSHM_ERROR(domid, sshm->id, "can only have one master.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+

[...]

tools/libxl/libxl_x86.c void libxl__arch_domain_build_info_acpi_setdefault( · line 603

Quoted 13 lines

tools/libxl/libxl_x86.c +5 -0

index 5f91fe4f92..450fb3d354 100644
@@ -596,6 +596,24 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    /* FIXME: Mark this as unsupported for calling p2m_add_foreign on two
+     * DomU's is currently not allowd on x86, see the comments in
+     * x86/mm/p2m.c: p2m_add_foreign */

Move the */ to a new line please. And add "." after "p2m_add_foreign".

Quoted 11 lines

tools/libxl/libxl_x86.c +11 -0

+     return false;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+    if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;

Quoted 8 lines

tools/libxl/libxl_x86.c +2 -1

+}
+
 /*
  * Local variables:
  * mode: C
--

2.14.2

Add libxl__sshm_del to unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.
* For a slave: unmap the shared pages, and cleanup related xs entries. decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.

Quoted 9 lines

Add libxl__sshm_del to unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.
* For a slave: unmap the shared pages, and cleanup related xs entries. decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.

This appears to be in line with what we discussed.

I would like to see some explanation for: if one or more of the things the code does fail half way, the system is still going to be in a consistent state. Most notably, there isn't going to be any page leaked with uncleaned refs.

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 185

Quoted 5 lines

tools/libxl/libxl_sshm.c +5 -0

+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+         isretry = true;

Indentation.

Hi Wei,

2017-11-01 23:55 GMT+08:00 Wei Liu <[email protected]>:

Quoted 11 lines · Zhongze Liu

Quoted 9 lines

Add libxl__sshm_del to unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.
* For a slave: unmap the shared pages, and cleanup related xs entries. decrease the refcount of the sshm region, if the refcount reaches 0, cleanup the whole sshm path.

Quoted 7 lines

This appears to be in line with what we discussed.

I would like to see some explanation for: if one or more of the things the code does fail half way, the system is still going to be in a consistent state. Most notably, there isn't going to be any page leaked with uncleaned refs.

The unmapping code is invoked during the domain destruction process, so we can't roll back. Everything done here is a "best effort": even if the code fails half way, what I could do is to report the errors and proceed anyway.

I might be totally wrong. So please correct me.

Quoted 6 lines

Quoted 5 lines

tools/libxl/libxl_sshm.c +5 -0

+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+         isretry = true;

Quoted 1 line

Indentation.

Cheers,

Zhongze Liu