From: Paul Durrant <xadimgnik@gmail.com>
To: "'Andrew Cooper'" <andrew.cooper3@citrix.com>,
"'Xen-devel'" <xen-devel@lists.xenproject.org>
Cc: "'George Dunlap'" <George.Dunlap@eu.citrix.com>,
"'Ian Jackson'" <iwj@xenproject.org>,
"'Jan Beulich'" <JBeulich@suse.com>,
"'Stefano Stabellini'" <sstabellini@kernel.org>,
"'Wei Liu'" <wl@xen.org>, "'Julien Grall'" <julien@xen.org>,
"'Michał Leszczyński'" <michal.leszczynski@cert.pl>,
"'Hubert Jasudowicz'" <hubert.jasudowicz@cert.pl>,
"'Tamas K Lengyel'" <tamas@tklengyel.com>
Subject: RE: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics
Date: Thu, 24 Sep 2020 11:06:43 +0100 [thread overview]
Message-ID: <003b01d6925a$67f9e2b0$37eda810$@xen.org> (raw)
In-Reply-To: <20200922182444.12350-5-andrew.cooper3@citrix.com>
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 22 September 2020 19:25
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <iwj@xenproject.org>; Jan Beulich <JBeulich@suse.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien Grall <julien@xen.org>; Paul Durrant
> <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz
> <hubert.jasudowicz@cert.pl>; Tamas K Lengyel <tamas@tklengyel.com>
> Subject: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics
>
> Calling XENMEM_acquire_resource with a NULL frame_list is a request for the
> size of the resource, but the returned 32 is bogus.
>
> If someone tries to follow it for XENMEM_resource_ioreq_server, the acquire
> call will fail as IOREQ servers currently top out at 2 frames, and it is only
> half the size of the default grant table limit for guests.
>
> Also, no users actually request a resource size, because it was never wired up
> in the sole implementaion of resource acquisition in Linux.
>
> Introduce a new resource_max_frames() to calculate the size of a resource, and
> implement it the IOREQ and grant subsystems.
>
> It is impossible to guarantee that a mapping call following a successful size
> call will succeed (e.g. The target IOREQ server gets destroyed, or the domain
> switches from grant v2 to v1). Document the restriction, and use the
> flexibility to simplify the paths to be lockless.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Ian Jackson <iwj@xenproject.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Paul Durrant <paul@xen.org>
> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
>
> v2:
> * Spelling fixes
> * Add more local variables.
> * Don't return any status frames on ARM where v2 support is compiled out.
> ---
> xen/arch/x86/mm.c | 20 ++++++++++++++++
> xen/common/grant_table.c | 23 ++++++++++++++++++
> xen/common/memory.c | 55 +++++++++++++++++++++++++++++++++----------
> xen/include/asm-x86/mm.h | 3 +++
> xen/include/public/memory.h | 16 +++++++++----
> xen/include/xen/grant_table.h | 8 +++++++
> xen/include/xen/mm.h | 6 +++++
> 7 files changed, 114 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d1cfc8fb4a..e82307bdae 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4591,6 +4591,26 @@ int xenmem_add_to_physmap_one(
> return rc;
> }
>
> +unsigned int arch_resource_max_frames(
> + struct domain *d, unsigned int type, unsigned int id)
> +{
> + unsigned int nr = 0;
> +
> + switch ( type )
> + {
> +#ifdef CONFIG_HVM
> + case XENMEM_resource_ioreq_server:
> + if ( !is_hvm_domain(d) )
> + break;
> + /* One frame for the buf-ioreq ring, and one frame per 128 vcpus. */
> + nr = 1 + DIV_ROUND_UP(d->max_vcpus * sizeof(struct ioreq), PAGE_SIZE);
The buf-ioreq ring is optional so a caller using this value may still get a resource acquisition failure unless the id is used to actually look up and check the ioreq server in question for the actual maximum. So this needs to call into a new function in ioreq.c.
Paul
> + break;
> +#endif
> + }
> +
> + return nr;
> +}
> +
> int arch_acquire_resource(struct domain *d, unsigned int type,
> unsigned int id, unsigned long frame,
> unsigned int nr_frames, xen_pfn_t mfn_list[])
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 912f07be47..8c401a5540 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,29 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
> return 0;
> }
>
> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id)
> +{
> + const struct grant_table *gt = d->grant_table;
> + unsigned int nr = 0;
> +
> + /* Don't need the grant lock. This limit is fixed at domain create time. */
> + switch ( id )
> + {
> + case XENMEM_resource_grant_table_id_shared:
> + nr = gt->max_grant_frames;
> + break;
> +
> + case XENMEM_resource_grant_table_id_status:
> + if ( GNTTAB_MAX_VERSION < 2 )
> + break;
> +
> + nr = grant_to_status_frames(gt->max_grant_frames);
> + break;
> + }
> +
> + return nr;
> +}
> +
> int gnttab_acquire_resource(
> struct domain *d, unsigned int id, unsigned long frame,
> unsigned int nr_frames, xen_pfn_t mfn_list[])
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 177fc378d9..c559935732 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,6 +1007,26 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> }
>
> +/*
> + * Return 0 on any kind of error. Caller converts to -EINVAL.
> + *
> + * All nonzero values should be repeatable (i.e. derived from some fixed
> + * property of the domain), and describe the full resource (i.e. mapping the
> + * result of this call will be the entire resource).
> + */
> +static unsigned int resource_max_frames(struct domain *d,
> + unsigned int type, unsigned int id)
> +{
> + switch ( type )
> + {
> + case XENMEM_resource_grant_table:
> + return gnttab_resource_max_frames(d, id);
> +
> + default:
> + return arch_resource_max_frames(d, type, id);
> + }
> +}
> +
> static int acquire_resource(
> XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> {
> @@ -1018,6 +1038,7 @@ static int acquire_resource(
> * use-cases then per-CPU arrays or heap allocations may be required.
> */
> xen_pfn_t mfn_list[32];
> + unsigned int max_frames;
> int rc;
>
> /*
> @@ -1034,19 +1055,6 @@ static int acquire_resource(
> if ( xmar.pad != 0 )
> return -EINVAL;
>
> - if ( guest_handle_is_null(xmar.frame_list) )
> - {
> - if ( xmar.nr_frames )
> - return -EINVAL;
> -
> - xmar.nr_frames = ARRAY_SIZE(mfn_list);
> -
> - if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> - return -EFAULT;
> -
> - return 0;
> - }
> -
> if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> return -E2BIG;
>
> @@ -1058,6 +1066,27 @@ static int acquire_resource(
> if ( rc )
> goto out;
>
> + max_frames = resource_max_frames(d, xmar.type, xmar.id);
> +
> + rc = -EINVAL;
> + if ( !max_frames )
> + goto out;
> +
> + if ( guest_handle_is_null(xmar.frame_list) )
> + {
> + if ( xmar.nr_frames )
> + goto out;
> +
> + xmar.nr_frames = max_frames;
> +
> + rc = -EFAULT;
> + if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> + goto out;
> +
> + rc = 0;
> + goto out;
> + }
> +
> switch ( xmar.type )
> {
> case XENMEM_resource_grant_table:
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index deeba75a1c..13977652a8 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -639,6 +639,9 @@ static inline bool arch_mfn_in_directmap(unsigned long mfn)
> return mfn <= (virt_to_mfn(eva - 1) + 1);
> }
>
> +unsigned int arch_resource_max_frames(struct domain *d,
> + unsigned int type, unsigned int id);
> +
> int arch_acquire_resource(struct domain *d, unsigned int type,
> unsigned int id, unsigned long frame,
> unsigned int nr_frames, xen_pfn_t mfn_list[]);
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 21d483298e..d7eb34f167 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -639,10 +639,18 @@ struct xen_mem_acquire_resource {
> #define XENMEM_resource_grant_table_id_status 1
>
> /*
> - * IN/OUT - As an IN parameter number of frames of the resource
> - * to be mapped. However, if the specified value is 0 and
> - * frame_list is NULL then this field will be set to the
> - * maximum value supported by the implementation on return.
> + * IN/OUT
> + *
> + * As an IN parameter number of frames of the resource to be mapped.
> + *
> + * When frame_list is NULL and nr_frames is 0, this is interpreted as a
> + * request for the size of the resource, which shall be returned in the
> + * nr_frames field.
> + *
> + * The size of a resource will never be zero, but a nonzero result doesn't
> + * guarantee that a subsequent mapping request will be successful. There
> + * are further type/id specific constraints which may change between the
> + * two calls.
> */
> uint32_t nr_frames;
> uint32_t pad;
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 5a2c75b880..bae4d79623 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -57,6 +57,8 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> mfn_t *mfn);
>
> +unsigned int gnttab_resource_max_frames(struct domain *d, unsigned int id);
> +
> int gnttab_acquire_resource(
> struct domain *d, unsigned int id, unsigned long frame,
> unsigned int nr_frames, xen_pfn_t mfn_list[]);
> @@ -93,6 +95,12 @@ static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
> return -EINVAL;
> }
>
> +static inline unsigned int gnttab_resource_max_frames(
> + struct domain *d, unsigned int id)
> +{
> + return 0;
> +}
> +
> static inline int gnttab_acquire_resource(
> struct domain *d, unsigned int id, unsigned long frame,
> unsigned int nr_frames, xen_pfn_t mfn_list[])
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 26a4a3d350..d686876b0e 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -686,6 +686,12 @@ static inline void put_page_alloc_ref(struct page_info *page)
> }
>
> #ifndef CONFIG_ARCH_ACQUIRE_RESOURCE
> +static inline unsigned int arch_resource_max_frames(
> + struct domain *d, unsigned int type, unsigned int id)
> +{
> + return 0;
> +}
> +
> static inline int arch_acquire_resource(
> struct domain *d, unsigned int type, unsigned int id, unsigned long frame,
> unsigned int nr_frames, xen_pfn_t mfn_list[])
> --
> 2.11.0
next prev parent reply other threads:[~2020-09-24 10:07 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-22 18:24 [PATCH v2 00/11] Multiple fixes to XENMEM_acquire_resource Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 01/11] xen/memory: Introduce CONFIG_ARCH_ACQUIRE_RESOURCE Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 02/11] xen/gnttab: Rework resource acquisition Andrew Cooper
2020-09-24 9:51 ` Paul Durrant
2021-01-11 21:22 ` Andrew Cooper
2021-01-12 8:23 ` Jan Beulich
2021-01-12 20:06 ` Andrew Cooper
2021-01-12 8:29 ` Paul Durrant
2020-09-25 13:17 ` Jan Beulich
2021-01-11 21:22 ` Andrew Cooper
2021-01-12 8:15 ` Jan Beulich
2021-01-12 18:11 ` Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 03/11] xen/memory: Fix compat XENMEM_acquire_resource for size requests Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics Andrew Cooper
2020-09-24 10:06 ` Paul Durrant [this message]
2020-09-24 10:57 ` Andrew Cooper
2020-09-24 11:04 ` Paul Durrant
2020-09-25 15:56 ` Jan Beulich
2020-09-22 18:24 ` [PATCH v2 05/11] tools/foreignmem: Support querying the size of a resource Andrew Cooper
2021-01-08 17:52 ` Andrew Cooper
2021-01-11 10:50 ` Roger Pau Monné
2021-01-11 15:00 ` Andrew Cooper
2021-01-11 15:26 ` [PATCH v3 " Andrew Cooper
2021-01-11 15:54 ` Roger Pau Monné
2020-09-22 18:24 ` [PATCH v2 06/11] xen/memory: Clarify the XENMEM_acquire_resource ABI description Andrew Cooper
2020-09-24 10:08 ` Paul Durrant
2020-09-22 18:24 ` [PATCH v2 07/11] xen/memory: Improve compat XENMEM_acquire_resource handling Andrew Cooper
2020-09-24 10:16 ` Paul Durrant
2020-09-28 9:09 ` Jan Beulich
2021-01-08 18:57 ` Andrew Cooper
2021-01-11 14:25 ` Jan Beulich
2020-09-22 18:24 ` [PATCH v2 08/11] xen/memory: Indent part of acquire_resource() Andrew Cooper
2020-09-24 10:36 ` Paul Durrant
2020-09-22 18:24 ` [PATCH v2 09/11] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource Andrew Cooper
2020-09-24 10:47 ` Paul Durrant
2021-01-08 19:36 ` Andrew Cooper
2020-09-28 9:37 ` Jan Beulich
2021-01-11 20:05 ` Andrew Cooper
2021-01-11 22:36 ` Andrew Cooper
2021-01-12 8:39 ` Jan Beulich
2020-09-22 18:24 ` [PATCH v2 10/11] TESTING dom0 Andrew Cooper
2020-09-22 18:24 ` [PATCH v2 11/11] TESTING XTF Andrew Cooper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='003b01d6925a$67f9e2b0$37eda810$@xen.org' \
--to=xadimgnik@gmail.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=hubert.jasudowicz@cert.pl \
--cc=iwj@xenproject.org \
--cc=julien@xen.org \
--cc=michal.leszczynski@cert.pl \
--cc=paul@xen.org \
--cc=sstabellini@kernel.org \
--cc=tamas@tklengyel.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.