[PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Julien Grall]

When creating a guest with more than 3GB of memory, the 2 banks will be
used and the loop with overrunning. The code will fail later on because
Xen will deny to populate the region:

domainbuilder: detail: xc_dom_devicetree_mem: called
domainbuilder: detail: xc_dom_mem_init: mem 3096 MB, pages 0xc1800 pages, 4k each
domainbuilder: detail: xc_dom_mem_init: 0xc1800 pages
domainbuilder: detail: xc_dom_boot_mem_init: called
domainbuilder: detail: set_mode: guest xen-3.0-aarch64, address size 64
domainbuilder: detail: xc_dom_malloc            : 14384 kB
domainbuilder: detail: populate_guest_memory: populating RAM @0000000040000000-0000000100000000 (3072MB)
domainbuilder: detail: populate_one_size: populated 0x3/0x3 entries with shift 18
domainbuilder: detail: populate_guest_memory: populating RAM @0000000200000000-0000000201800000 (24MB)
domainbuilder: detail: populate_one_size: populated 0xc/0xc entries with shift 9
domainbuilder: detail: populate_guest_memory: populating RAM @0000007fad41c000-0007fb39dd42c000 (2141954816MB)
domainbuilder: detail: populate_one_size: populated 0x100/0x1e4 entries with shift 0
domainbuilder: detail: populate_guest_memory: Not enough RAM

This is because we are currently accessing the bank before checking the
validity of the index. AFAICT, on  Debian Jessie, the compiler (gcc 4.9.2) is
assuming that it's not necessary to verify the index because it's used
before. This is a valid assumption because the operand of && are
execute from from left to right.

Re-order the checks to verify the validity of the index before accessing
the bank.

The problem has been present since the introduction of the multi-bank
feature in commit 45d9867837f099e9eed4189dac5ed39d1fe2ed49 " tools: arm:
prepare domain builder for multiple banks of guest RAM".

---
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>

This patch is a candidate for Xen 4.6 and backport to Xen 4.5. Without
it, it's impossible to boot guest using more than 3GB (limit after which
the memory bank is used).
---
 tools/libxc/xc_dom_arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index b00d667..aeaba54 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -460,7 +460,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
         dom->p2m_host[pfn] = INVALID_P2M_ENTRY;
 
     /* setup initial p2m and allocate guest memory */
-    for ( i = 0; dom->rambank_size[i] && i < GUEST_RAM_BANKS; i++ )
+    for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
     {
         if ((rc = populate_guest_memory(dom,
                                         bankbase[i] >> XC_PAGE_SHIFT,
-- 
2.1.4

Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Julien Grall]

On 17/09/15 18:36, Julien Grall wrote:
> When creating a guest with more than 3GB of memory, the 2 banks will be
> used and the loop with overrunning. The code will fail later on because
> Xen will deny to populate the region:
> 
> domainbuilder: detail: xc_dom_devicetree_mem: called
> domainbuilder: detail: xc_dom_mem_init: mem 3096 MB, pages 0xc1800 pages, 4k each
> domainbuilder: detail: xc_dom_mem_init: 0xc1800 pages
> domainbuilder: detail: xc_dom_boot_mem_init: called
> domainbuilder: detail: set_mode: guest xen-3.0-aarch64, address size 64
> domainbuilder: detail: xc_dom_malloc            : 14384 kB
> domainbuilder: detail: populate_guest_memory: populating RAM @0000000040000000-0000000100000000 (3072MB)
> domainbuilder: detail: populate_one_size: populated 0x3/0x3 entries with shift 18
> domainbuilder: detail: populate_guest_memory: populating RAM @0000000200000000-0000000201800000 (24MB)
> domainbuilder: detail: populate_one_size: populated 0xc/0xc entries with shift 9
> domainbuilder: detail: populate_guest_memory: populating RAM @0000007fad41c000-0007fb39dd42c000 (2141954816MB)
> domainbuilder: detail: populate_one_size: populated 0x100/0x1e4 entries with shift 0
> domainbuilder: detail: populate_guest_memory: Not enough RAM
> 
> This is because we are currently accessing the bank before checking the
> validity of the index. AFAICT, on  Debian Jessie, the compiler (gcc 4.9.2) is
> assuming that it's not necessary to verify the index because it's used
> before. This is a valid assumption because the operand of && are
> execute from from left to right.
> 
> Re-order the checks to verify the validity of the index before accessing
> the bank.
> 
> The problem has been present since the introduction of the multi-bank
> feature in commit 45d9867837f099e9eed4189dac5ed39d1fe2ed49 " tools: arm:
> prepare domain builder for multiple banks of guest RAM".

Hmmmm I forgot my Signed-off-by :(.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

> ---
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> This patch is a candidate for Xen 4.6 and backport to Xen 4.5. Without
> it, it's impossible to boot guest using more than 3GB (limit after which
> the memory bank is used).
> ---
>  tools/libxc/xc_dom_arm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index b00d667..aeaba54 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -460,7 +460,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>          dom->p2m_host[pfn] = INVALID_P2M_ENTRY;
>  
>      /* setup initial p2m and allocate guest memory */
> -    for ( i = 0; dom->rambank_size[i] && i < GUEST_RAM_BANKS; i++ )
> +    for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
>      {
>          if ((rc = populate_guest_memory(dom,
>                                          bankbase[i] >> XC_PAGE_SHIFT,
> 


-- 
Julien Grall

Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Ian Campbell]

On Thu, 2015-09-17 at 18:42 +0100, Julien Grall wrote:
> On 17/09/15 18:36, Julien Grall wrote:
> > When creating a guest with more than 3GB of memory, the 2 banks will be
> > used and the loop with overrunning. The code will fail later on because
> > Xen will deny to populate the region:
> > 
> > domainbuilder: detail: xc_dom_devicetree_mem: called
> > domainbuilder: detail: xc_dom_mem_init: mem 3096 MB, pages 0xc1800
> > pages, 4k each
> > domainbuilder: detail: xc_dom_mem_init: 0xc1800 pages
> > domainbuilder: detail: xc_dom_boot_mem_init: called
> > domainbuilder: detail: set_mode: guest xen-3.0-aarch64, address size 64
> > domainbuilder: detail: xc_dom_malloc            : 14384 kB
> > domainbuilder: detail: populate_guest_memory: populating RAM
> > @0000000040000000-0000000100000000 (3072MB)
> > domainbuilder: detail: populate_one_size: populated 0x3/0x3 entries
> > with shift 18
> > domainbuilder: detail: populate_guest_memory: populating RAM
> > @0000000200000000-0000000201800000 (24MB)
> > domainbuilder: detail: populate_one_size: populated 0xc/0xc entries
> > with shift 9
> > domainbuilder: detail: populate_guest_memory: populating RAM
> > @0000007fad41c000-0007fb39dd42c000 (2141954816MB)
> > domainbuilder: detail: populate_one_size: populated 0x100/0x1e4 entries
> > with shift 0
> > domainbuilder: detail: populate_guest_memory: Not enough RAM
> > 
> > This is because we are currently accessing the bank before checking the
> > validity of the index. AFAICT, on  Debian Jessie, the compiler (gcc
> > 4.9.2) is
> > assuming that it's not necessary to verify the index because it's used
> > before. This is a valid assumption because the operand of && are
> > execute from from left to right.
> > 
> > Re-order the checks to verify the validity of the index before
> > accessing
> > the bank.
> > 
> > The problem has been present since the introduction of the multi-bank
> > feature in commit 45d9867837f099e9eed4189dac5ed39d1fe2ed49 " tools:
> > arm:
> > prepare domain builder for multiple banks of guest RAM".
> 
> Hmmmm I forgot my Signed-off-by :(.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

This is a no brainer for 4.6 (and further) IMHO and with Wei not being
around I shall plan to apply later today unless there are objections.

Ian.

> 
> > ---
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > This patch is a candidate for Xen 4.6 and backport to Xen 4.5. Without
> > it, it's impossible to boot guest using more than 3GB (limit after
> > which
> > the memory bank is used).
> > ---
> >  tools/libxc/xc_dom_arm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> > index b00d667..aeaba54 100644
> > --- a/tools/libxc/xc_dom_arm.c
> > +++ b/tools/libxc/xc_dom_arm.c
> > @@ -460,7 +460,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >          dom->p2m_host[pfn] = INVALID_P2M_ENTRY;
> >  
> >      /* setup initial p2m and allocate guest memory */
> > -    for ( i = 0; dom->rambank_size[i] && i < GUEST_RAM_BANKS; i++ )
> > +    for ( i = 0; i < GUEST_RAM_BANKS && dom->rambank_size[i]; i++ )
> >      {
> >          if ((rc = populate_guest_memory(dom,
> >                                          bankbase[i] >> XC_PAGE_SHIFT,
> > 
> 
> 

Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Wei Liu]

On Fri, Sep 18, 2015 at 09:52:44AM +0100, Ian Campbell wrote:
> On Thu, 2015-09-17 at 18:42 +0100, Julien Grall wrote:
> > On 17/09/15 18:36, Julien Grall wrote:
> > > When creating a guest with more than 3GB of memory, the 2 banks will be
> > > used and the loop with overrunning. The code will fail later on because
> > > Xen will deny to populate the region:
> > > 
> > > domainbuilder: detail: xc_dom_devicetree_mem: called
> > > domainbuilder: detail: xc_dom_mem_init: mem 3096 MB, pages 0xc1800
> > > pages, 4k each
> > > domainbuilder: detail: xc_dom_mem_init: 0xc1800 pages
> > > domainbuilder: detail: xc_dom_boot_mem_init: called
> > > domainbuilder: detail: set_mode: guest xen-3.0-aarch64, address size 64
> > > domainbuilder: detail: xc_dom_malloc            : 14384 kB
> > > domainbuilder: detail: populate_guest_memory: populating RAM
> > > @0000000040000000-0000000100000000 (3072MB)
> > > domainbuilder: detail: populate_one_size: populated 0x3/0x3 entries
> > > with shift 18
> > > domainbuilder: detail: populate_guest_memory: populating RAM
> > > @0000000200000000-0000000201800000 (24MB)
> > > domainbuilder: detail: populate_one_size: populated 0xc/0xc entries
> > > with shift 9
> > > domainbuilder: detail: populate_guest_memory: populating RAM
> > > @0000007fad41c000-0007fb39dd42c000 (2141954816MB)
> > > domainbuilder: detail: populate_one_size: populated 0x100/0x1e4 entries
> > > with shift 0
> > > domainbuilder: detail: populate_guest_memory: Not enough RAM
> > > 
> > > This is because we are currently accessing the bank before checking the
> > > validity of the index. AFAICT, on  Debian Jessie, the compiler (gcc
> > > 4.9.2) is
> > > assuming that it's not necessary to verify the index because it's used
> > > before. This is a valid assumption because the operand of && are
> > > execute from from left to right.
> > > 
> > > Re-order the checks to verify the validity of the index before
> > > accessing
> > > the bank.
> > > 
> > > The problem has been present since the introduction of the multi-bank
> > > feature in commit 45d9867837f099e9eed4189dac5ed39d1fe2ed49 " tools:
> > > arm:
> > > prepare domain builder for multiple banks of guest RAM".
> > 
> > Hmmmm I forgot my Signed-off-by :(.
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> This is a no brainer for 4.6 (and further) IMHO and with Wei not being
> around I shall plan to apply later today unless there are objections.
> 

No objection from me of course.

Wei.

Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Ian Campbell]

On Fri, 2015-09-18 at 11:30 +0100, Wei Liu wrote:

> > > Hmmmm I forgot my Signed-off-by :(.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > This is a no brainer for 4.6 (and further) IMHO and with Wei not being
> > around I shall plan to apply later today unless there are objections.
> > 
> 
> No objection from me of course.

Applied to staging and staging-4.6.

Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Ian Campbell]

On Mon, 2015-09-21 at 12:54 +0100, Ian Campbell wrote:
> On Fri, 2015-09-18 at 11:30 +0100, Wei Liu wrote:
> 
> > > > Hmmmm I forgot my Signed-off-by :(.
> > > > 
> > > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > This is a no brainer for 4.6 (and further) IMHO and with Wei not
> > > being
> > > around I shall plan to apply later today unless there are objections.
> > > 
> > 
> > No objection from me of course.
> 
> Applied to staging and staging-4.6.

This turned out to be a fib, I'd only applied it to staging.

I've made a note to apply to 4.6 once the current moratorium is over.

Ian, this should go into 4.5 too.

Ian.


> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for-4.6] tools/libxc: arm: Check the index before accessing the bank

[Ian Campbell]

On Fri, 2015-09-25 at 13:26 +0100, Ian Campbell wrote:
> On Mon, 2015-09-21 at 12:54 +0100, Ian Campbell wrote:
> > On Fri, 2015-09-18 at 11:30 +0100, Wei Liu wrote:
> > 
> > > > > Hmmmm I forgot my Signed-off-by :(.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > > > 
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > 
> > > > This is a no brainer for 4.6 (and further) IMHO and with Wei not
> > > > being
> > > > around I shall plan to apply later today unless there are
> > > > objections.
> > > > 
> > > 
> > > No objection from me of course.
> > 
> > Applied to staging and staging-4.6.
> 
> This turned out to be a fib, I'd only applied it to staging.
> 
> I've made a note to apply to 4.6 once the current moratorium is over.

Done.