[PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Andrew Cooper]

It is not used, and can cause a spurious failure of the set_gdt() hypercall in
low memory situations.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/mm.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 63aa666..4b76587 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4439,20 +4439,15 @@ long set_gdt(struct vcpu *v,
     l1_pgentry_t *pl1e;
     /* NB. There are 512 8-byte entries per GDT page. */
     int i, nr_pages = (entries + 511) / 512;
-    unsigned long *pfns;
 
     if ( entries > FIRST_RESERVED_GDT_ENTRY )
         return -EINVAL;
 
-    pfns = xmalloc_array(unsigned long, nr_pages);
-    if ( !pfns )
-        return -ENOMEM;
-
     /* Check the pages in the new GDT. */
     for ( i = 0; i < nr_pages; i++ )
     {
         struct page_info *page;
-        pfns[i] = frames[i];
+
         page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
         if ( !page )
             goto fail;
@@ -4476,7 +4471,6 @@ long set_gdt(struct vcpu *v,
         l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
     }
 
-    xfree(pfns);
     return 0;
 
  fail:
@@ -4484,7 +4478,6 @@ long set_gdt(struct vcpu *v,
     {
         put_page_and_type(mfn_to_page(frames[i]));
     }
-    xfree(pfns);
     return -EINVAL;
 }
 
-- 
1.7.10.4

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Wei Liu]

On Mon, Aug 03, 2015 at 06:05:43PM +0100, Andrew Cooper wrote:
> It is not used, and can cause a spurious failure of the set_gdt() hypercall in
> low memory situations.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> ---

This array does appear to be write-only and never gets read.

FWIW

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

It's obviously correct and fixes a problem, so it can be applied to 4.6
tree.

>  xen/arch/x86/mm.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 63aa666..4b76587 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4439,20 +4439,15 @@ long set_gdt(struct vcpu *v,
>      l1_pgentry_t *pl1e;
>      /* NB. There are 512 8-byte entries per GDT page. */
>      int i, nr_pages = (entries + 511) / 512;
> -    unsigned long *pfns;
>  
>      if ( entries > FIRST_RESERVED_GDT_ENTRY )
>          return -EINVAL;
>  
> -    pfns = xmalloc_array(unsigned long, nr_pages);
> -    if ( !pfns )
> -        return -ENOMEM;
> -
>      /* Check the pages in the new GDT. */
>      for ( i = 0; i < nr_pages; i++ )
>      {
>          struct page_info *page;
> -        pfns[i] = frames[i];
> +
>          page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
>          if ( !page )
>              goto fail;
> @@ -4476,7 +4471,6 @@ long set_gdt(struct vcpu *v,
>          l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
>      }
>  
> -    xfree(pfns);
>      return 0;
>  
>   fail:
> @@ -4484,7 +4478,6 @@ long set_gdt(struct vcpu *v,
>      {
>          put_page_and_type(mfn_to_page(frames[i]));
>      }
> -    xfree(pfns);
>      return -EINVAL;
>  }
>  
> -- 
> 1.7.10.4

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Ian Campbell]

On Tue, 2015-08-04 at 10:34 +0100, Wei Liu wrote:
> On Mon, Aug 03, 2015 at 06:05:43PM +0100, Andrew Cooper wrote:
> > It is not used, and can cause a spurious failure of the set_gdt() 
> > hypercall in
> > low memory situations.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Jan Beulich <JBeulich@suse.com>
> > ---
> 
> This array does appear to be write-only and never gets read.
> 
> FWIW
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

The use was removed by:

commit 5889a3e1d123bdad4a3d150310d647db55459966
Author: Tim Deegan <tim@xen.org>
Date:   Thu May 17 10:24:54 2012 +0100

    x86/mm: Use get_page_from_gfn() instead of get_gfn()/put_gfn.
    
    Signed-off-by: Tim Deegan <tim@xen.org>
    Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

after it was introduced by:

commit 51032ca058e43fbd37ea1f7c7c003496f6451340
Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Date:   Fri Nov 11 18:11:34 2011 +0000

    Modify naming of queries into the p2m
    [...]
    Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
    Acked-by: Tim Deegan <tim@xen.org>
    Committed-by: Keir Fraser <keir@xen.org>

> It's obviously correct and fixes a problem, so it can be applied to 4.6
> tree.

FWIW I think so too, but I'm hesitant to apply without a maintainer(/Jan)'s
ack. I think this can probably wait until next week? 

> >  xen/arch/x86/mm.c |    9 +--------

George is the current "X86 MEMORY MANAGEMENT" maintainer, which is
xen/arch/x86/mm/*, should xen/arch/x86/mm.c be included there too?

> >  1 file changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 63aa666..4b76587 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4439,20 +4439,15 @@ long set_gdt(struct vcpu *v,
> >      l1_pgentry_t *pl1e;
> >      /* NB. There are 512 8-byte entries per GDT page. */
> >      int i, nr_pages = (entries + 511) / 512;
> > -    unsigned long *pfns;
> >  
> >      if ( entries > FIRST_RESERVED_GDT_ENTRY )
> >          return -EINVAL;
> >  
> > -    pfns = xmalloc_array(unsigned long, nr_pages);
> > -    if ( !pfns )
> > -        return -ENOMEM;
> > -
> >      /* Check the pages in the new GDT. */
> >      for ( i = 0; i < nr_pages; i++ )
> >      {
> >          struct page_info *page;
> > -        pfns[i] = frames[i];
> > +
> >          page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
> >          if ( !page )
> >              goto fail;
> > @@ -4476,7 +4471,6 @@ long set_gdt(struct vcpu *v,
> >          l1e_write(&pl1e[i], l1e_from_pfn(frames[i], 
> > __PAGE_HYPERVISOR_RW));
> >      }
> >  
> > -    xfree(pfns);
> >      return 0;
> >  
> >   fail:
> > @@ -4484,7 +4478,6 @@ long set_gdt(struct vcpu *v,
> >      {
> >          put_page_and_type(mfn_to_page(frames[i]));
> >      }
> > -    xfree(pfns);
> >      return -EINVAL;
> >  }
> >  

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Andrew Cooper]

On 04/08/15 14:59, Ian Campbell wrote:
> On Tue, 2015-08-04 at 10:34 +0100, Wei Liu wrote:
>> On Mon, Aug 03, 2015 at 06:05:43PM +0100, Andrew Cooper wrote:
>>> It is not used, and can cause a spurious failure of the set_gdt() 
>>> hypercall in
>>> low memory situations.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> ---
>> This array does appear to be write-only and never gets read.
>>
>> FWIW
>>
>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> The use was removed by:
>
> commit 5889a3e1d123bdad4a3d150310d647db55459966
> Author: Tim Deegan <tim@xen.org>
> Date:   Thu May 17 10:24:54 2012 +0100
>
>     x86/mm: Use get_page_from_gfn() instead of get_gfn()/put_gfn.
>     
>     Signed-off-by: Tim Deegan <tim@xen.org>
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> after it was introduced by:
>
> commit 51032ca058e43fbd37ea1f7c7c003496f6451340
> Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Date:   Fri Nov 11 18:11:34 2011 +0000
>
>     Modify naming of queries into the p2m
>     [...]
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>     Acked-by: Tim Deegan <tim@xen.org>
>     Committed-by: Keir Fraser <keir@xen.org>
>
>> It's obviously correct and fixes a problem, so it can be applied to 4.6
>> tree.
> FWIW I think so too, but I'm hesitant to apply without a maintainer(/Jan)'s
> ack. I think this can probably wait until next week? 
>
>>>  xen/arch/x86/mm.c |    9 +--------
> George is the current "X86 MEMORY MANAGEMENT" maintainer, which is
> xen/arch/x86/mm/*, should xen/arch/x86/mm.c be included there too?

Oops - I hadn't even twigged.

Despite being in mm.c, this really isn't memory management and is just
general x86 behaviour, which is covered by my maintership role.

Not that I expect anyone (maintainer or not) to have a problem with this
change.

FWIW, following the Linux LDT thread, I have a bucket load of changes to
do with GDT and LDT handling for PV guests.  While most of that is
definitely 4.7 material, I felt that this fix alone should be fixed in 4.6

~Andrew

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Tim Deegan]

At 14:59 +0100 on 04 Aug (1438700361), Ian Campbell wrote:
> On Tue, 2015-08-04 at 10:34 +0100, Wei Liu wrote:
> > On Mon, Aug 03, 2015 at 06:05:43PM +0100, Andrew Cooper wrote:
> > > It is not used, and can cause a spurious failure of the set_gdt() 
> > > hypercall in
> > > low memory situations.
> > > 
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > ---
> > > CC: Jan Beulich <JBeulich@suse.com>
> > > ---
> > 
> > This array does appear to be write-only and never gets read.
> > 
> > FWIW
> > 
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> The use was removed by:
> 
> commit 5889a3e1d123bdad4a3d150310d647db55459966
> Author: Tim Deegan <tim@xen.org>
> Date:   Thu May 17 10:24:54 2012 +0100
> 
>     x86/mm: Use get_page_from_gfn() instead of get_gfn()/put_gfn.
>     
>     Signed-off-by: Tim Deegan <tim@xen.org>
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

Yep, clearly an oversight in this patch.

> after it was introduced by:
> 
> commit 51032ca058e43fbd37ea1f7c7c003496f6451340
> Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Date:   Fri Nov 11 18:11:34 2011 +0000
> 
>     Modify naming of queries into the p2m
>     [...]
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>     Acked-by: Tim Deegan <tim@xen.org>
>     Committed-by: Keir Fraser <keir@xen.org>
> 
> > It's obviously correct and fixes a problem, so it can be applied to 4.6
> > tree.

+1, FWIW. 

Cheers,

Tim.

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Jan Beulich]

>>> On 04.08.15 at 15:59, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-08-04 at 10:34 +0100, Wei Liu wrote:
>> On Mon, Aug 03, 2015 at 06:05:43PM +0100, Andrew Cooper wrote:
>> > It is not used, and can cause a spurious failure of the set_gdt() 
>> > hypercall in
>> > low memory situations.
>> > 
>> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> > ---
>> > CC: Jan Beulich <JBeulich@suse.com>
>> > ---
>> 
>> This array does appear to be write-only and never gets read.
>> 
>> FWIW
>> 
>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> The use was removed by:
> 
> commit 5889a3e1d123bdad4a3d150310d647db55459966
> Author: Tim Deegan <tim@xen.org>
> Date:   Thu May 17 10:24:54 2012 +0100
> 
>     x86/mm: Use get_page_from_gfn() instead of get_gfn()/put_gfn.
>     
>     Signed-off-by: Tim Deegan <tim@xen.org>
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> after it was introduced by:
> 
> commit 51032ca058e43fbd37ea1f7c7c003496f6451340
> Author: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Date:   Fri Nov 11 18:11:34 2011 +0000
> 
>     Modify naming of queries into the p2m
>     [...]
>     Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>     Acked-by: Tim Deegan <tim@xen.org>
>     Committed-by: Keir Fraser <keir@xen.org>

Indeed pointing out the commit that introduced the unused array
right in the commit message would have helped both in review and
in determination whether (and to which branches) to backport.

Jan

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[George Dunlap]

On Mon, Aug 3, 2015 at 6:05 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> It is not used, and can cause a spurious failure of the set_gdt() hypercall in
> low memory situations.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/mm.c |    9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 63aa666..4b76587 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4439,20 +4439,15 @@ long set_gdt(struct vcpu *v,
>      l1_pgentry_t *pl1e;
>      /* NB. There are 512 8-byte entries per GDT page. */
>      int i, nr_pages = (entries + 511) / 512;
> -    unsigned long *pfns;
>
>      if ( entries > FIRST_RESERVED_GDT_ENTRY )
>          return -EINVAL;
>
> -    pfns = xmalloc_array(unsigned long, nr_pages);
> -    if ( !pfns )
> -        return -ENOMEM;
> -
>      /* Check the pages in the new GDT. */
>      for ( i = 0; i < nr_pages; i++ )
>      {
>          struct page_info *page;
> -        pfns[i] = frames[i];
> +
>          page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
>          if ( !page )
>              goto fail;
> @@ -4476,7 +4471,6 @@ long set_gdt(struct vcpu *v,
>          l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
>      }
>
> -    xfree(pfns);
>      return 0;
>
>   fail:
> @@ -4484,7 +4478,6 @@ long set_gdt(struct vcpu *v,
>      {
>          put_page_and_type(mfn_to_page(frames[i]));
>      }
> -    xfree(pfns);
>      return -EINVAL;
>  }
>
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Ian Campbell]

On Tue, 2015-08-04 at 15:17 +0100, George Dunlap wrote:
> On Mon, Aug 3, 2015 at 6:05 PM, Andrew Cooper <andrew.cooper3@citrix.com> 
> wrote:
> > It is not used, and can cause a spurious failure of the set_gdt() 
> > hypercall in
> > low memory situations.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

It's unclear from MAINTAINERS if this is formally enough for the patch to
be applied but under the circumstances (pretty obvious patch, Reviewed-by
more than one person, including the X86 MEMORY MANAGEMENT maintainer even
if mm.c isn't strictly under that for some reason) I've added my own
Revieed-by and applied, hopefully that is ok.

Perhaps there should be a patch to MAINTAINERS to add arch/x86/mm.c to the
relevant subsection? (Or maybe the file is just horribly named?)

Ian.

Re: [PATCH for-4.6] x86/gdt: Drop write-only, xalloc()'d array from set_gdt()

[Jan Beulich]

>>> On 05.08.15 at 11:58, <ian.campbell@citrix.com> wrote:
> Perhaps there should be a patch to MAINTAINERS to add arch/x86/mm.c to the
> relevant subsection? (Or maybe the file is just horribly named?)

Yeah, the file, while mostly about (PV) memory management, holds
more than just that; mm/ otoh is pretty properly limited to (mostly
HVM) memory management. I think a nice cleanup item would be to
split mm.c and move much of it into mm/, which would then eliminate
the need for the adjustment you name.

Jan