| Message ID | 20231006091353.96367-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | domain: followup for phys address mapping series | expand |
Hi Roger, > On Oct 6, 2023, at 17:13, Roger Pau Monne <roger.pau@citrix.com> wrote: > > unmap_domain_page_global() expects the provided address to be page aligned, or > else some of the called functions will trigger assertions, like > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. > > The following assert has been reported by osstest arm 32bit tests: > > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 > (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- > (XEN) CPU: 0 > (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c > [...] > (XEN) Xen call trace: > (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) > (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) > (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 > (XEN) [<00208e38>] unmap_guest_area+0x90/0xec > (XEN) [<00208f98>] domain_kill+0x104/0x180 > (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc > (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c > (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 > > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > --- > unmap_domain_page_global() and vunmap() should likely have the same alignment > asserts, as not all paths lead to detecting the misalignment of the provided > linear address. Will do a separate patch. > --- > xen/common/domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index b8281d7cff9d..2dcc64e659cc 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) > > if ( pg ) > { > - unmap_domain_page_global(map); > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > put_page_and_type(pg); > } > } > -- > 2.42.0 >
Hi Roger, On 06/10/2023 10:13, Roger Pau Monne wrote: > unmap_domain_page_global() expects the provided address to be page aligned, or > else some of the called functions will trigger assertions, like > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. > > The following assert has been reported by osstest arm 32bit tests: > > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 > (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- > (XEN) CPU: 0 > (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c > [...] > (XEN) Xen call trace: > (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) > (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) > (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 > (XEN) [<00208e38>] unmap_guest_area+0x90/0xec > (XEN) [<00208f98>] domain_kill+0x104/0x180 > (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc > (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c > (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 > > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > unmap_domain_page_global() and vunmap() should likely have the same alignment > asserts, as not all paths lead to detecting the misalignment of the provided > linear address. Will do a separate patch. unmap_domain_page() seems to be able to deal with unaligned pointer. And supporting unaligned pointer in vunmap()/unmap_domain_page_global() would simplifyy call to those functions. So I would consider to deal with the alignment rather than adding ASSERT() in the two functions. destroy_xen_mappings() and modify_xen_mappings() should stay as-is though. Anyway, I don't think this is a 4.18 material. > --- > xen/common/domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/domain.c b/xen/common/domain.c > index b8281d7cff9d..2dcc64e659cc 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) > > if ( pg ) > { > - unmap_domain_page_global(map); > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); Looking at the code, I can't find where 'map' may have become unaligned. Do you have a pointer? Depending on the answer, the call in map_guest_area() may also need some adjustment. Cheers,
On Fri, Oct 06, 2023 at 11:08:09AM +0100, Julien Grall wrote: > Hi Roger, > > On 06/10/2023 10:13, Roger Pau Monne wrote: > > unmap_domain_page_global() expects the provided address to be page aligned, or > > else some of the called functions will trigger assertions, like > > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. > > > > The following assert has been reported by osstest arm 32bit tests: > > > > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 > > (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- > > (XEN) CPU: 0 > > (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c > > [...] > > (XEN) Xen call trace: > > (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) > > (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) > > (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 > > (XEN) [<00208e38>] unmap_guest_area+0x90/0xec > > (XEN) [<00208f98>] domain_kill+0x104/0x180 > > (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc > > (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c > > (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 > > > > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > unmap_domain_page_global() and vunmap() should likely have the same alignment > > asserts, as not all paths lead to detecting the misalignment of the provided > > linear address. Will do a separate patch. > > unmap_domain_page() seems to be able to deal with unaligned pointer. And > supporting unaligned pointer in vunmap()/unmap_domain_page_global() would > simplifyy call to those functions. > > So I would consider to deal with the alignment rather than adding ASSERT() > in the two functions. destroy_xen_mappings() and modify_xen_mappings() > should stay as-is though. > > Anyway, I don't think this is a 4.18 material. Maybe, I don't really have a strong opinion. It seems more sane to me to expect a page aligned linear address if the function is unmapping a page, leaves less room for misuse IMO. > > --- > > xen/common/domain.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/xen/common/domain.c b/xen/common/domain.c > > index b8281d7cff9d..2dcc64e659cc 100644 > > --- a/xen/common/domain.c > > +++ b/xen/common/domain.c > > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) > > if ( pg ) > > { > > - unmap_domain_page_global(map); > > + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); > > Looking at the code, I can't find where 'map' may have become unaligned. Do > you have a pointer? In map_guest_area(), there's: if ( ~gaddr ) /* Map (i.e. not just unmap)? */ { [...] map = __map_domain_page_global(pg); if ( !map ) { put_page_and_type(pg); return -ENOMEM; } map += PAGE_OFFSET(gaddr); } > Depending on the answer, the call in map_guest_area() may also need some > adjustment. Indeed, I've missed that one, let me spin v2. Thanks, Roger.
diff --git a/xen/common/domain.c b/xen/common/domain.c index b8281d7cff9d..2dcc64e659cc 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area) if ( pg ) { - unmap_domain_page_global(map); + unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK)); put_page_and_type(pg); } }
unmap_domain_page_global() expects the provided address to be page aligned, or else some of the called functions will trigger assertions, like modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm. The following assert has been reported by osstest arm 32bit tests: (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243 (XEN) ----[ Xen-4.18-rc arm32 debug=y Not tainted ]---- (XEN) CPU: 0 (XEN) PC: 00271a38 destroy_xen_mappings+0x50/0x5c [...] (XEN) Xen call trace: (XEN) [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC) (XEN) [<00235aa8>] vunmap+0x30/0x1a0 (LR) (XEN) [<0026ad88>] unmap_domain_page_global+0x10/0x20 (XEN) [<00208e38>] unmap_guest_area+0x90/0xec (XEN) [<00208f98>] domain_kill+0x104/0x180 (XEN) [<00239e3c>] do_domctl+0x8ac/0x14fc (XEN) [<0027ae34>] do_trap_guest_sync+0x570/0x66c (XEN) [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4 Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- unmap_domain_page_global() and vunmap() should likely have the same alignment asserts, as not all paths lead to detecting the misalignment of the provided linear address. Will do a separate patch. --- xen/common/domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)