diff mbox series

[v3,1/7] xen/gnttab: Rework resource acquisition

Message ID 20210112194841.1537-2-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series Multiple fixes to XENMEM_acquire_resource | expand

Commit Message

Andrew Cooper Jan. 12, 2021, 7:48 p.m. UTC
The existing logic doesn't function in the general case for mapping a guests
grant table, due to arbitrary 32 frame limit, and the default grant table
limit being 64.

In order to start addressing this, rework the existing grant table logic by
implementing a single gnttab_acquire_resource().  This is far more efficient
than the previous acquire_grant_table() in memory.c because it doesn't take
the grant table write lock, and attempt to grow the table, for every single
frame.

The new gnttab_acquire_resource() function subsumes the previous two
gnttab_get_{shared,status}_frame() helpers.

No functional change.

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>

v3:
 * Fold switch statements in gnttab_acquire_resource()
---
 xen/common/grant_table.c      | 80 ++++++++++++++++++++++++++++---------------
 xen/common/memory.c           | 42 ++---------------------
 xen/include/xen/grant_table.h | 19 ++++------
 3 files changed, 62 insertions(+), 79 deletions(-)

Comments

Jan Beulich Jan. 15, 2021, 11:43 a.m. UTC | #1
On 12.01.2021 20:48, Andrew Cooper wrote:
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
> 
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.
> 
> The new gnttab_acquire_resource() function subsumes the previous two
> gnttab_get_{shared,status}_frame() helpers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit with a remark and a question:

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,59 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
>  
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;

It doesn't look like this initializer was needed. The only
use of i that I can spot is the loop near the end, which
starts from 0.

> +    mfn_t tmp;
> +    void **vaddrs;
> +    int rc;
> +
> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;

Can't these two be folded into

    unsigned int tot_frames = frame + nr_frames;

    if ( tot_frames < frame )
        return -EINVAL;

? Both truncation and wrapping look to be taken care of this
way.

Jan
Jan Beulich Jan. 15, 2021, 11:56 a.m. UTC | #2
On 12.01.2021 20:48, Andrew Cooper wrote:
> The existing logic doesn't function in the general case for mapping a guests
> grant table, due to arbitrary 32 frame limit, and the default grant table
> limit being 64.
> 
> In order to start addressing this, rework the existing grant table logic by
> implementing a single gnttab_acquire_resource().  This is far more efficient
> than the previous acquire_grant_table() in memory.c because it doesn't take
> the grant table write lock, and attempt to grow the table, for every single
> frame.
> 
> The new gnttab_acquire_resource() function subsumes the previous two
> gnttab_get_{shared,status}_frame() helpers.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'm sorry, but I have to withdraw the R-b given a few minutes ago.

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,59 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>      return 0;
>  }
>  
> +int gnttab_acquire_resource(
> +    struct domain *d, unsigned int id, unsigned long frame,
> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
> +{
> +    struct grant_table *gt = d->grant_table;
> +    unsigned int i = nr_frames, tot_frames;
> +    mfn_t tmp;
> +    void **vaddrs;
> +    int rc;
> +
> +    /* Overflow checks */
> +    if ( frame + nr_frames < frame )
> +        return -EINVAL;
> +
> +    tot_frames = frame + nr_frames;
> +    if ( tot_frames != frame + nr_frames )
> +        return -EINVAL;

While you did remove the explicit returning of an error when
nr_frames is zero, ...

> +    /* Grow table if necessary. */
> +    grant_write_lock(gt);
> +    rc = -EINVAL;
> +    switch ( id )
> +    {
> +    case XENMEM_resource_grant_table_id_shared:
> +        vaddrs = gt->shared_raw;
> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);

... this will degenerate (and still cause an error) when frame
is also zero, and will cause undue growing of the table when
frame is non-zero yet not overly large.

As indicated before, I'm of the clear opinion that here - like
elsewhere - a number of zero frames requested means that no
action be taken at all, and success be returned.

> +        break;
> +
> +    case XENMEM_resource_grant_table_id_status:
> +        if ( gt->gt_version != 2 )
> +            break;

As per various other places in this file I think you want to
wrap this in evaluate_nospec().

Jan
Andrew Cooper Jan. 15, 2021, 4:03 p.m. UTC | #3
On 15/01/2021 11:43, Jan Beulich wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,59 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>      return 0;
>>  }
>>  
>> +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
> It doesn't look like this initializer was needed. The only
> use of i that I can spot is the loop near the end, which
> starts from 0.

Its possibly stale from v1.  I'll adjust.

>> +    mfn_t tmp;
>> +    void **vaddrs;
>> +    int rc;
>> +
>> +    /* Overflow checks */
>> +    if ( frame + nr_frames < frame )
>> +        return -EINVAL;
>> +
>> +    tot_frames = frame + nr_frames;
>> +    if ( tot_frames != frame + nr_frames )
>> +        return -EINVAL;
> Can't these two be folded into
>
>     unsigned int tot_frames = frame + nr_frames;
>
>     if ( tot_frames < frame )
>         return -EINVAL;
>
> ? Both truncation and wrapping look to be taken care of this
> way.

Not when frame is a multiple of 4G (or fractionally over, I think).

This ABI with mismatched widths really is obnoxious to work with.

~Andrew
Jan Beulich Jan. 15, 2021, 4:26 p.m. UTC | #4
On 15.01.2021 17:03, Andrew Cooper wrote:
> On 15/01/2021 11:43, Jan Beulich wrote:
>>> +    mfn_t tmp;
>>> +    void **vaddrs;
>>> +    int rc;
>>> +
>>> +    /* Overflow checks */
>>> +    if ( frame + nr_frames < frame )
>>> +        return -EINVAL;
>>> +
>>> +    tot_frames = frame + nr_frames;
>>> +    if ( tot_frames != frame + nr_frames )
>>> +        return -EINVAL;
>> Can't these two be folded into
>>
>>     unsigned int tot_frames = frame + nr_frames;
>>
>>     if ( tot_frames < frame )
>>         return -EINVAL;
>>
>> ? Both truncation and wrapping look to be taken care of this
>> way.
> 
> Not when frame is a multiple of 4G (or fractionally over, I think).

How that? In this case any unsigned int value will be less than
frame.

Jan
Jan Beulich Jan. 15, 2021, 4:27 p.m. UTC | #5
On 15.01.2021 17:26, Jan Beulich wrote:
> On 15.01.2021 17:03, Andrew Cooper wrote:
>> On 15/01/2021 11:43, Jan Beulich wrote:
>>>> +    mfn_t tmp;
>>>> +    void **vaddrs;
>>>> +    int rc;
>>>> +
>>>> +    /* Overflow checks */
>>>> +    if ( frame + nr_frames < frame )
>>>> +        return -EINVAL;
>>>> +
>>>> +    tot_frames = frame + nr_frames;
>>>> +    if ( tot_frames != frame + nr_frames )
>>>> +        return -EINVAL;
>>> Can't these two be folded into
>>>
>>>     unsigned int tot_frames = frame + nr_frames;
>>>
>>>     if ( tot_frames < frame )
>>>         return -EINVAL;
>>>
>>> ? Both truncation and wrapping look to be taken care of this
>>> way.
>>
>> Not when frame is a multiple of 4G (or fractionally over, I think).
> 
> How that? In this case any unsigned int value will be less than
> frame.

And in the 32-bit case the above becomes a simple overflow
check.

Jan
Andrew Cooper Jan. 15, 2021, 4:57 p.m. UTC | #6
On 15/01/2021 11:56, Jan Beulich wrote:
>> +    /* Grow table if necessary. */
>> +    grant_write_lock(gt);
>> +    rc = -EINVAL;
>> +    switch ( id )
>> +    {
>> +    case XENMEM_resource_grant_table_id_shared:
>> +        vaddrs = gt->shared_raw;
>> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> ... this will degenerate (and still cause an error) when frame
> is also zero, and will cause undue growing of the table when
> frame is non-zero yet not overly large.

Urgh, yes - that is why I had the check.

In which case I retract my change between v2 and v3 here.

> As indicated before, I'm of the clear opinion that here - like
> elsewhere - a number of zero frames requested means that no
> action be taken at all, and success be returned.

The general world we work in (POSIX) agrees with my opinion over yours
when it comes to this matter.

I spent a lot of time and effort getting this logic correct in v2, and I
do not have any further time to waste adding complexity to support a
non-existent corner case, nor is it reasonable to further delay all the
work which is depending on this series.  This entire mess is already too
damn complicated, without taking extra complexity.

Entertaining the idea of supporting 0 length requests is really not as
simple as you seem to think it is, and is a large part of why I'm
stubbornly refusing to do so.

I am going to commit this patch (with some of the other minor adjustments).

If you wish to add the extra complexity yourself, you are welcome to all
the unit tests I put together which exercise the complexity, but I will
not ack the resulting change.

~Andrew
Jan Beulich Jan. 18, 2021, 8:23 a.m. UTC | #7
On 15.01.2021 17:57, Andrew Cooper wrote:
> On 15/01/2021 11:56, Jan Beulich wrote:
>>> +    /* Grow table if necessary. */
>>> +    grant_write_lock(gt);
>>> +    rc = -EINVAL;
>>> +    switch ( id )
>>> +    {
>>> +    case XENMEM_resource_grant_table_id_shared:
>>> +        vaddrs = gt->shared_raw;
>>> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
>> ... this will degenerate (and still cause an error) when frame
>> is also zero, and will cause undue growing of the table when
>> frame is non-zero yet not overly large.
> 
> Urgh, yes - that is why I had the check.
> 
> In which case I retract my change between v2 and v3 here.
> 
>> As indicated before, I'm of the clear opinion that here - like
>> elsewhere - a number of zero frames requested means that no
>> action be taken at all, and success be returned.
> 
> The general world we work in (POSIX) agrees with my opinion over yours
> when it comes to this matter.

I assume you are referring to mmap()? I ask because I think there
are numerous counter examples (some even in the C standard):
malloc() & friends allow for either behavior. memcpy() / memmove()
happily do nothing when passed a zero size. read() / write()
are at least allowed to read/write nothing (and return success)
when told so. Otoh I notice that a zero vector count passed to
readv() / writev() is indeed an error, yet nothing is said at all
about individual vector elements specifying zero size.

Plus of course I don't think POSIX is the main reference point
here, when the rest of the hypercalls allowing for some form of
batching permit empty batches.

> I spent a lot of time and effort getting this logic correct in v2, and I
> do not have any further time to waste adding complexity to support a
> non-existent corner case, nor is it reasonable to further delay all the
> work which is depending on this series.  This entire mess is already too
> damn complicated, without taking extra complexity.
> 
> Entertaining the idea of supporting 0 length requests is really not as
> simple as you seem to think it is, and is a large part of why I'm
> stubbornly refusing to do so.

I'd be really happy to be educated of the complications; sadly
so far you've only claimed ones would exist without actually
going into sufficient detail. In particular I don't view placing 

    if ( size == 0 )
        return 0;

suitably early coming anywhere near "complexity". Even more so
that as per your reply you mean to undo removal of a respective
check, just that in your version it'll return an error instead
of success.

> I am going to commit this patch (with some of the other minor adjustments).

I'm not concerned enough of the introduced inconsistency to
outright veto you doing so, but I still don't think this is an
appropriate step to take under the present conditions.

Jan
Andrew Cooper Jan. 28, 2021, 10:56 p.m. UTC | #8
On 18/01/2021 08:23, Jan Beulich wrote:
> On 15.01.2021 17:57, Andrew Cooper wrote:
>> On 15/01/2021 11:56, Jan Beulich wrote:
>>>> +    /* Grow table if necessary. */
>>>> +    grant_write_lock(gt);
>>>> +    rc = -EINVAL;
>>>> +    switch ( id )
>>>> +    {
>>>> +    case XENMEM_resource_grant_table_id_shared:
>>>> +        vaddrs = gt->shared_raw;
>>>> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
>>> ... this will degenerate (and still cause an error) when frame
>>> is also zero, and will cause undue growing of the table when
>>> frame is non-zero yet not overly large.
>> Urgh, yes - that is why I had the check.
>>
>> In which case I retract my change between v2 and v3 here.
>>
>>> As indicated before, I'm of the clear opinion that here - like
>>> elsewhere - a number of zero frames requested means that no
>>> action be taken at all, and success be returned.
>> The general world we work in (POSIX) agrees with my opinion over yours
>> when it comes to this matter.
> I assume you are referring to mmap()? I ask because I think there
> are numerous counter examples (some even in the C standard):
> malloc() & friends allow for either behavior. memcpy() / memmove()
> ...

This entire infrastructure lives behind an mmap() (or equivalent) system
call.  Other specs are not relevant.

Any request for a zero sized mapping is a bug.  It is either a buggy
caller, or buggy continuation logic.

>> I spent a lot of time and effort getting this logic correct in v2, and I
>> do not have any further time to waste adding complexity to support a
>> non-existent corner case, nor is it reasonable to further delay all the
>> work which is depending on this series.  This entire mess is already too
>> damn complicated, without taking extra complexity.
>>
>> Entertaining the idea of supporting 0 length requests is really not as
>> simple as you seem to think it is, and is a large part of why I'm
>> stubbornly refusing to do so.
> I'd be really happy to be educated of the complications; sadly
> so far you've only claimed ones would exist without actually
> going into sufficient detail. In particular I don't view placing 
>
>     if ( size == 0 )
>         return 0;
>
> suitably early coming anywhere near "complexity". Even more so
> that as per your reply you mean to undo removal of a respective
> check, just that in your version it'll return an error instead
> of success.

I am not being a belligerent arse for the sake of it.  I've got far
better things I ought to be doing with my time.

I spent a lot of time getting this working, and with sufficient testing
to demonstrate it working in practice, particularly in continuation cases.

You've spent a lot of time and effort insisting that I reintroduce
support a fundamentally broken corner case, despite my best attempts to
demonstrate why it will livelock in the hypervisor because of the mess
which is the double looping between the compat and native handlers.

You've also spent a lot of time obfuscating the overflow checks and
breaking them in the process.

You are welcome, in your own time - and not mine, to use the testing
infrastructure I've already provided to discover why the compat mess has
a habit of livelocking in certain continuation cases.  (It won't
actually livelock if you switch to using return 0.  You'll instead hit
the ASSERT_UNREACHABLE() I put in a failsafe path specifically to avoid
bugs in this are turning back into XSAs.)

Combined with the fact that 0 length requests are buggy in all
circumstances, I chose to implement logic which is robust even in the
case of failure, because the compat logic is almost intractable and
borderline untestable because noone runs 32bit kernels in anger these
days.  I can't commit my test infrastructure which spots the problems,
because we obviously can't have a hypercall which panics when the input
buffer doesn't match the test pattern.

I am not inclined to adopt an approach which is fundamentally more
likely to contain security vulnerabilities in a fragile and untestable
area of code.  You are going to have to come up with a far more
compelling argument that "because I want to support 0 length mapping
requests" to change my mind.

~Andrew
Jan Beulich Jan. 29, 2021, 9:33 a.m. UTC | #9
On 28.01.2021 23:56, Andrew Cooper wrote:
> On 18/01/2021 08:23, Jan Beulich wrote:
>> On 15.01.2021 17:57, Andrew Cooper wrote:
>>> On 15/01/2021 11:56, Jan Beulich wrote:
>>>>> +    /* Grow table if necessary. */
>>>>> +    grant_write_lock(gt);
>>>>> +    rc = -EINVAL;
>>>>> +    switch ( id )
>>>>> +    {
>>>>> +    case XENMEM_resource_grant_table_id_shared:
>>>>> +        vaddrs = gt->shared_raw;
>>>>> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
>>>> ... this will degenerate (and still cause an error) when frame
>>>> is also zero, and will cause undue growing of the table when
>>>> frame is non-zero yet not overly large.
>>> Urgh, yes - that is why I had the check.
>>>
>>> In which case I retract my change between v2 and v3 here.
>>>
>>>> As indicated before, I'm of the clear opinion that here - like
>>>> elsewhere - a number of zero frames requested means that no
>>>> action be taken at all, and success be returned.
>>> The general world we work in (POSIX) agrees with my opinion over yours
>>> when it comes to this matter.
>> I assume you are referring to mmap()? I ask because I think there
>> are numerous counter examples (some even in the C standard):
>> malloc() & friends allow for either behavior. memcpy() / memmove()
>> ...
> 
> This entire infrastructure lives behind an mmap() (or equivalent) system
> call.  Other specs are not relevant.

With this you're implying restrictions on what callers might
use this for. I don't see why a ring-0 only environment
would necessarily have anything like mmap().

Anyway, I'm not going to further comment on any of the below.
I'm not going to either ack or nak this change, so if you
have the agreement of others feel free to put in.

Jan

> Any request for a zero sized mapping is a bug.  It is either a buggy
> caller, or buggy continuation logic.
> 
>>> I spent a lot of time and effort getting this logic correct in v2, and I
>>> do not have any further time to waste adding complexity to support a
>>> non-existent corner case, nor is it reasonable to further delay all the
>>> work which is depending on this series.  This entire mess is already too
>>> damn complicated, without taking extra complexity.
>>>
>>> Entertaining the idea of supporting 0 length requests is really not as
>>> simple as you seem to think it is, and is a large part of why I'm
>>> stubbornly refusing to do so.
>> I'd be really happy to be educated of the complications; sadly
>> so far you've only claimed ones would exist without actually
>> going into sufficient detail. In particular I don't view placing 
>>
>>     if ( size == 0 )
>>         return 0;
>>
>> suitably early coming anywhere near "complexity". Even more so
>> that as per your reply you mean to undo removal of a respective
>> check, just that in your version it'll return an error instead
>> of success.
> 
> I am not being a belligerent arse for the sake of it.  I've got far
> better things I ought to be doing with my time.
> 
> I spent a lot of time getting this working, and with sufficient testing
> to demonstrate it working in practice, particularly in continuation cases.
> 
> You've spent a lot of time and effort insisting that I reintroduce
> support a fundamentally broken corner case, despite my best attempts to
> demonstrate why it will livelock in the hypervisor because of the mess
> which is the double looping between the compat and native handlers.
> 
> You've also spent a lot of time obfuscating the overflow checks and
> breaking them in the process.
> 
> You are welcome, in your own time - and not mine, to use the testing
> infrastructure I've already provided to discover why the compat mess has
> a habit of livelocking in certain continuation cases.  (It won't
> actually livelock if you switch to using return 0.  You'll instead hit
> the ASSERT_UNREACHABLE() I put in a failsafe path specifically to avoid
> bugs in this are turning back into XSAs.)
> 
> Combined with the fact that 0 length requests are buggy in all
> circumstances, I chose to implement logic which is robust even in the
> case of failure, because the compat logic is almost intractable and
> borderline untestable because noone runs 32bit kernels in anger these
> days.  I can't commit my test infrastructure which spots the problems,
> because we obviously can't have a hypercall which panics when the input
> buffer doesn't match the test pattern.
> 
> I am not inclined to adopt an approach which is fundamentally more
> likely to contain security vulnerabilities in a fragile and untestable
> area of code.  You are going to have to come up with a far more
> compelling argument that "because I want to support 0 length mapping
> requests" to change my mind.
> 
> ~Andrew
>
Ian Jackson Jan. 29, 2021, 5:44 p.m. UTC | #10
Jan Beulich writes ("Re: [PATCH v3 1/7] xen/gnttab: Rework resource acquisition"):
> Anyway, I'm not going to further comment on any of the below.
> I'm not going to either ack or nak this change, so if you
> have the agreement of others feel free to put in.

This patch,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff mbox series

Patch

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a5d3ed8bda..f560c250d7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4013,6 +4013,59 @@  static int gnttab_get_shared_frame_mfn(struct domain *d,
     return 0;
 }
 
+int gnttab_acquire_resource(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[])
+{
+    struct grant_table *gt = d->grant_table;
+    unsigned int i = nr_frames, tot_frames;
+    mfn_t tmp;
+    void **vaddrs;
+    int rc;
+
+    /* Overflow checks */
+    if ( frame + nr_frames < frame )
+        return -EINVAL;
+
+    tot_frames = frame + nr_frames;
+    if ( tot_frames != frame + nr_frames )
+        return -EINVAL;
+
+    /* Grow table if necessary. */
+    grant_write_lock(gt);
+    rc = -EINVAL;
+    switch ( id )
+    {
+    case XENMEM_resource_grant_table_id_shared:
+        vaddrs = gt->shared_raw;
+        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
+        break;
+
+    case XENMEM_resource_grant_table_id_status:
+        if ( gt->gt_version != 2 )
+            break;
+
+        /* Check that void ** is a suitable representation for gt->status. */
+        BUILD_BUG_ON(!__builtin_types_compatible_p(
+                         typeof(gt->status), grant_status_t **));
+        vaddrs = (void **)gt->status;
+        rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
+        break;
+    }
+
+    /* Any errors?  Bad id, or from growing the table? */
+    if ( rc )
+        goto out;
+
+    for ( i = 0; i < nr_frames; ++i )
+        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
+
+ out:
+    grant_write_unlock(gt);
+
+    return rc;
+}
+
 int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
 {
     int rc = 0;
@@ -4047,33 +4100,6 @@  int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
     return rc;
 }
 
-int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn)
-{
-    struct grant_table *gt = d->grant_table;
-    int rc;
-
-    grant_write_lock(gt);
-    rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
-    grant_write_unlock(gt);
-
-    return rc;
-}
-
-int gnttab_get_status_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn)
-{
-    struct grant_table *gt = d->grant_table;
-    int rc;
-
-    grant_write_lock(gt);
-    rc = (gt->gt_version == 2) ?
-        gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
-    grant_write_unlock(gt);
-
-    return rc;
-}
-
 static void gnttab_usage_print(struct domain *rd)
 {
     int first = 1;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index b21b6c452d..82cf7b41ee 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1052,44 +1052,6 @@  static long xatp_permission_check(struct domain *d, unsigned int space)
     return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
 }
 
-static int acquire_grant_table(struct domain *d, unsigned int id,
-                               unsigned long frame,
-                               unsigned int nr_frames,
-                               xen_pfn_t mfn_list[])
-{
-    unsigned int i = nr_frames;
-
-    /* Iterate backwards in case table needs to grow */
-    while ( i-- != 0 )
-    {
-        mfn_t mfn = INVALID_MFN;
-        int rc;
-
-        switch ( id )
-        {
-        case XENMEM_resource_grant_table_id_shared:
-            rc = gnttab_get_shared_frame(d, frame + i, &mfn);
-            break;
-
-        case XENMEM_resource_grant_table_id_status:
-            rc = gnttab_get_status_frame(d, frame + i, &mfn);
-            break;
-
-        default:
-            rc = -EINVAL;
-            break;
-        }
-
-        if ( rc )
-            return rc;
-
-        ASSERT(!mfn_eq(mfn, INVALID_MFN));
-        mfn_list[i] = mfn_x(mfn);
-    }
-
-    return 0;
-}
-
 static int acquire_resource(
     XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
 {
@@ -1144,8 +1106,8 @@  static int acquire_resource(
     switch ( xmar.type )
     {
     case XENMEM_resource_grant_table:
-        rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
-                                 mfn_list);
+        rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
+                                     mfn_list);
         break;
 
     default:
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 98603604b8..5a2c75b880 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -56,10 +56,10 @@  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);
-int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn);
-int gnttab_get_status_frame(struct domain *d, unsigned long idx,
-                            mfn_t *mfn);
+
+int gnttab_acquire_resource(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[]);
 
 #else
 
@@ -93,14 +93,9 @@  static inline int gnttab_map_frame(struct domain *d, unsigned long idx,
     return -EINVAL;
 }
 
-static inline int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
-                                          mfn_t *mfn)
-{
-    return -EINVAL;
-}
-
-static inline int gnttab_get_status_frame(struct domain *d, unsigned long idx,
-                                          mfn_t *mfn)
+static inline int gnttab_acquire_resource(
+    struct domain *d, unsigned int id, unsigned long frame,
+    unsigned int nr_frames, xen_pfn_t mfn_list[])
 {
     return -EINVAL;
 }