[PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[Ian Campbell]

Jan points out that processing the list backwards is rather counter intuitive
and that the effect of the hypercall can differ between forwards and backwards
processing (e.g. in the presence of duplicate idx or gpfn, which would be
unusualy but as Jan says, users are a creative bunch)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
---
v2: Remember to crank the errs array too.

    Resending despite abuse of overwriting the IN paramters, since coming to
    rely on backwards processing is a harder hole to get out of than going
    from modifying the strict to not.

Release: subtle ABI change, should go in to 4.4 before people rely on it (they
are not relying on it today TTBOMK and it seems unlikely but lets not risk it)
---
 xen/arch/arm/mm.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e235364..f1b7099 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1062,21 +1062,18 @@ static int xenmem_add_to_physmap_range(struct domain *d,
 {
     int rc;
 
-    /* Process entries in reverse order to allow continuations */
     while ( xatpr->size > 0 )
     {
         xen_ulong_t idx;
         xen_pfn_t gpfn;
 
-        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs,
-                                             xatpr->size-1, 1)) )
+        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) )
         {
             rc = -EFAULT;
             goto out;
         }
 
-        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns,
-                                             xatpr->size-1, 1)) )
+        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) )
         {
             rc = -EFAULT;
             goto out;
@@ -1086,8 +1083,7 @@ static int xenmem_add_to_physmap_range(struct domain *d,
                                        xatpr->foreign_domid,
                                        idx, gpfn);
 
-        if ( unlikely(copy_to_guest_offset(xatpr->errs,
-                                           xatpr->size-1, &rc, 1)) )
+        if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) )
         {
             rc = -EFAULT;
             goto out;
@@ -1096,6 +1092,9 @@ static int xenmem_add_to_physmap_range(struct domain *d,
         if ( rc < 0 )
             goto out;
 
+        guest_handle_add_offset(xatpr->idxs, 1);
+        guest_handle_add_offset(xatpr->gpfns, 1);
+        guest_handle_add_offset(xatpr->errs, 1);
         xatpr->size--;
 
         /* Check for continuation if it's not the last interation */
-- 
1.7.10.4

Re: [PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[Jan Beulich]

>>> On 18.12.13 at 14:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> Jan points out that processing the list backwards is rather counter intuitive
> and that the effect of the hypercall can differ between forwards and 
> backwards
> processing (e.g. in the presence of duplicate idx or gpfn, which would be
> unusualy but as Jan says, users are a creative bunch)
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
> v2: Remember to crank the errs array too.
> 
>     Resending despite abuse of overwriting the IN paramters, since coming to
>     rely on backwards processing is a harder hole to get out of than going
>     from modifying the strict to not.
> 
> Release: subtle ABI change, should go in to 4.4 before people rely on it 
> (they
> are not relying on it today TTBOMK and it seems unlikely but lets not risk 
> it)
> ---
>  xen/arch/arm/mm.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index e235364..f1b7099 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1062,21 +1062,18 @@ static int xenmem_add_to_physmap_range(struct domain 
> *d,
>  {
>      int rc;
>  
> -    /* Process entries in reverse order to allow continuations */
>      while ( xatpr->size > 0 )
>      {
>          xen_ulong_t idx;
>          xen_pfn_t gpfn;
>  
> -        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs,
> -                                             xatpr->size-1, 1)) )
> +        if ( unlikely(copy_from_guest_offset(&idx, xatpr->idxs, 0, 1)) )
>          {
>              rc = -EFAULT;
>              goto out;
>          }
>  
> -        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns,
> -                                             xatpr->size-1, 1)) )
> +        if ( unlikely(copy_from_guest_offset(&gpfn, xatpr->gpfns, 0, 1)) )
>          {
>              rc = -EFAULT;
>              goto out;
> @@ -1086,8 +1083,7 @@ static int xenmem_add_to_physmap_range(struct domain 
> *d,
>                                         xatpr->foreign_domid,
>                                         idx, gpfn);
>  
> -        if ( unlikely(copy_to_guest_offset(xatpr->errs,
> -                                           xatpr->size-1, &rc, 1)) )
> +        if ( unlikely(copy_to_guest_offset(xatpr->errs, 0, &rc, 1)) )
>          {
>              rc = -EFAULT;
>              goto out;
> @@ -1096,6 +1092,9 @@ static int xenmem_add_to_physmap_range(struct domain 
> *d,
>          if ( rc < 0 )
>              goto out;
>  
> +        guest_handle_add_offset(xatpr->idxs, 1);
> +        guest_handle_add_offset(xatpr->gpfns, 1);
> +        guest_handle_add_offset(xatpr->errs, 1);
>          xatpr->size--;
>  
>          /* Check for continuation if it's not the last interation */
> -- 
> 1.7.10.4

Re: [PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[George Dunlap]

On 12/18/2013 01:39 PM, Ian Campbell wrote:
> Jan points out that processing the list backwards is rather counter intuitive
> and that the effect of the hypercall can differ between forwards and backwards
> processing (e.g. in the presence of duplicate idx or gpfn, which would be
> unusualy but as Jan says, users are a creative bunch)
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
> v2: Remember to crank the errs array too.
>
>      Resending despite abuse of overwriting the IN paramters, since coming to
>      rely on backwards processing is a harder hole to get out of than going
>      from modifying the strict to not.
>
> Release: subtle ABI change, should go in to 4.4 before people rely on it (they
> are not relying on it today TTBOMK and it seems unlikely but lets not risk it)

Yes, I agree: the risk of having to work around a bad ABI indefinitely 
is worse than the relatively small risk of finding a bug in this and 
then fixing it.

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

  -George

Re: [PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[Ian Campbell]

On Wed, 2013-12-18 at 14:08 +0000, Jan Beulich wrote:
> >>> On 18.12.13 at 14:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> > Jan points out that processing the list backwards is rather counter intuitive
> > and that the effect of the hypercall can differ between forwards and 
> > backwards
> > processing (e.g. in the presence of duplicate idx or gpfn, which would be
> > unusualy but as Jan says, users are a creative bunch)
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks. I've applied without waiting for a further ARM maintainer ack
since the nature of the change is not really specifc to arm

(I fixed the "unusualy" typo I spotted above as I went).

Just to confirm -- Are you going to make this not writeback its params
while you fix the x86 equivalent? I can take a look if not.

Re: [PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[Jan Beulich]

>>> On 18.12.13 at 15:37, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2013-12-18 at 14:08 +0000, Jan Beulich wrote:
>> >>> On 18.12.13 at 14:39, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > Jan points out that processing the list backwards is rather counter 
> intuitive
>> > and that the effect of the hypercall can differ between forwards and 
>> > backwards
>> > processing (e.g. in the presence of duplicate idx or gpfn, which would be
>> > unusualy but as Jan says, users are a creative bunch)
>> > 
>> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> 
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks. I've applied without waiting for a further ARM maintainer ack
> since the nature of the change is not really specifc to arm
> 
> (I fixed the "unusualy" typo I spotted above as I went).
> 
> Just to confirm -- Are you going to make this not writeback its params
> while you fix the x86 equivalent? I can take a look if not.

You've seen the patches meanwhile I guess - the answer is yes,
but by way of making the framework code common rather than
fixing it in both ARM and x86 separately.

Jan

Re: [PATCH v2] xen: arm: process XENMEM_add_to_physmap_range forwards not backwards.

[Ian Campbell]

On Wed, 2013-12-18 at 15:02 +0000, Jan Beulich wrote:
> >>> On 18.12.13 at 15:37, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2013-12-18 at 14:08 +0000, Jan Beulich wrote:
> >> >>> On 18.12.13 at 14:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > Jan points out that processing the list backwards is rather counter 
> > intuitive
> >> > and that the effect of the hypercall can differ between forwards and 
> >> > backwards
> >> > processing (e.g. in the presence of duplicate idx or gpfn, which would be
> >> > unusualy but as Jan says, users are a creative bunch)
> >> > 
> >> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >> 
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Thanks. I've applied without waiting for a further ARM maintainer ack
> > since the nature of the change is not really specifc to arm
> > 
> > (I fixed the "unusualy" typo I spotted above as I went).
> > 
> > Just to confirm -- Are you going to make this not writeback its params
> > while you fix the x86 equivalent? I can take a look if not.
> 
> You've seen the patches meanwhile I guess - the answer is yes,

Yes, I saw, thanks!

> but by way of making the framework code common rather than
> fixing it in both ARM and x86 separately.

That certainly makes sense.

Ian.