On 15/12/2020 16:25, Jan Beulich wrote:
> Drop a bogus ASSERT() - we don't typically assert incoming domain
> pointers to be non-NULL, and there's no particular reason to do so here.
>
> Replace the open-coded DOMID_SELF check by use of
> rcu_lock_remote_domain_by_id(), at the same time covering the request
> being made with the current domain's actual ID.
>
> Move the "both domains same" check into just the path where it really
> is meaningful.
>
> Swap the order of the two puts, such that
> - the p2m lock isn't needlessly held across put_page(),
> - a separate put_page() on an error path can be avoided,
> - they're inverse to the order of the respective gets.
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
Acked-by: Andrew Cooper <andrew.coop...@citrix.com>
> ---
> The DOMID_SELF check being converted also suggests to me that there's an
> implication of tdom == current->domain, which would in turn appear to
> mean the "both domains same" check could as well be dropped altogether.
I don't see anything conceptually wrong with the toolstack creating a
foreign mapping on behalf of a guest at construction time. I'd go as
far as to argue that it is an interface shortcoming if this didn't
function correctly.
>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2560,9 +2560,6 @@ int p2m_add_foreign(struct domain *tdom,
> int rc;
> struct domain *fdom;
>
> - ASSERT(tdom);
> - if ( foreigndom == DOMID_SELF )
> - return -EINVAL;
> /*
> * hvm fixme: until support is added to p2m teardown code to cleanup any
> * foreign entries, limit this to hardware domain only.
> @@ -2573,13 +2570,15 @@ int p2m_add_foreign(struct domain *tdom,
> if ( foreigndom == DOMID_XEN )
> fdom = rcu_lock_domain(dom_xen);
> else
> - fdom = rcu_lock_domain_by_id(foreigndom);
> - if ( fdom == NULL )
> - return -ESRCH;
> + {
> + rc = rcu_lock_remote_domain_by_id(foreigndom, &fdom);
It occurs to me that rcu_lock_remote_domain_by_id()'s self error path
ought to be -EINVAL rather than -EPERM. It's never for permissions
reasons that we restrict to remote domains like this - always for
technical ones.
But that is definitely content for a different patch.
~Andrew
