Message ID | 1460322896-1115-8-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Sun, Apr 10, 2016 at 10:14 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > From: Andrew Cooper <andrew.cooper3@citrix.com> > > To simply change the permissions on existing Xen mappings. The existing > destroy_xen_mappings() is altered to support a change the PTE permissions. > > A new destroy_xen_mappings() is introduced, as the special case of not passing > _PAGE_PRESENT to modify_xen_mappings(). This part looks correct to me. > As cleanup (and an ideal functional test), the boot logic which remaps Xen's > code and data with reduced permissions is altered to use > modify_xen_mappings(), rather than map_pages_to_xen() passing the same mfn's > back in. This seems like a very good idea; I've just got one comment... > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 22dc148..b867242 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1220,23 +1220,19 @@ void __init noreturn __start_xen(unsigned long mbi_p) > if ( !using_2M_mapping() ) > { > /* Mark .text as RX (avoiding the first 2M superpage). */ > - map_pages_to_xen(XEN_VIRT_START + MB(2), > - PFN_DOWN(__pa(XEN_VIRT_START + MB(2))), > - PFN_DOWN(__2M_text_end - > - (const char *)(XEN_VIRT_START + MB(2))), > - PAGE_HYPERVISOR_RX); > + modify_xen_mappings(XEN_VIRT_START + MB(2), > + (unsigned long)&__2M_text_end, > + _PAGE_PRESENT); > > /* Mark .rodata as RO. */ > - map_pages_to_xen((unsigned long)&__2M_rodata_start, > - PFN_DOWN(__pa(__2M_rodata_start)), > - PFN_DOWN(__2M_rodata_end - __2M_rodata_start), > - PAGE_HYPERVISOR_RO); > + modify_xen_mappings((unsigned long)&__2M_rodata_start, > + (unsigned long)&__2M_rodata_end, > + _PAGE_NX | _PAGE_PRESENT); > > /* Mark .data and .bss as RW. */ > - map_pages_to_xen((unsigned long)&__2M_rwdata_start, > - PFN_DOWN(__pa(__2M_rwdata_start)), > - PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start), > - PAGE_HYPERVISOR_RW); > + modify_xen_mappings((unsigned long)&__2M_rwdata_start, > + (unsigned long)&__2M_rwdata_end, > + _PAGE_NX | _PAGE_RW | _PAGE_PRESENT); The only thing here is that you're changing from the centrally-defined PAGE_HYPERVISOR_$TYPE flags to what might be called "magic combinations" of flags. I realize this is because modify_xen_mappings() only allows you to pass RW, PRESENT, and NX, but it's still somewhat sub-optimal. Alternatives that come to mind: 1. Mask out the other bits at the caller (i.e., PAGE_HYPERVISOR_RX & MAP_PAGES_MASK) 2. Have modify_xen_mappings() filter out the bits it doesn't want to change 3. Extend modify_xen_mappings() to be able to modify other bits. 4. Have modify_xen_mappings() assert that the only bits which are *changing* are the FLAGS_MASK bits Thoughts? -George
On 11/04/16 13:51, George Dunlap wrote: > On Sun, Apr 10, 2016 at 10:14 PM, Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: >> From: Andrew Cooper <andrew.cooper3@citrix.com> >> >> To simply change the permissions on existing Xen mappings. The existing >> destroy_xen_mappings() is altered to support a change the PTE permissions. >> >> A new destroy_xen_mappings() is introduced, as the special case of not passing >> _PAGE_PRESENT to modify_xen_mappings(). > This part looks correct to me. > >> As cleanup (and an ideal functional test), the boot logic which remaps Xen's >> code and data with reduced permissions is altered to use >> modify_xen_mappings(), rather than map_pages_to_xen() passing the same mfn's >> back in. > This seems like a very good idea; I've just got one comment... > >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 22dc148..b867242 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1220,23 +1220,19 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> if ( !using_2M_mapping() ) >> { >> /* Mark .text as RX (avoiding the first 2M superpage). */ >> - map_pages_to_xen(XEN_VIRT_START + MB(2), >> - PFN_DOWN(__pa(XEN_VIRT_START + MB(2))), >> - PFN_DOWN(__2M_text_end - >> - (const char *)(XEN_VIRT_START + MB(2))), >> - PAGE_HYPERVISOR_RX); >> + modify_xen_mappings(XEN_VIRT_START + MB(2), >> + (unsigned long)&__2M_text_end, >> + _PAGE_PRESENT); >> >> /* Mark .rodata as RO. */ >> - map_pages_to_xen((unsigned long)&__2M_rodata_start, >> - PFN_DOWN(__pa(__2M_rodata_start)), >> - PFN_DOWN(__2M_rodata_end - __2M_rodata_start), >> - PAGE_HYPERVISOR_RO); >> + modify_xen_mappings((unsigned long)&__2M_rodata_start, >> + (unsigned long)&__2M_rodata_end, >> + _PAGE_NX | _PAGE_PRESENT); >> >> /* Mark .data and .bss as RW. */ >> - map_pages_to_xen((unsigned long)&__2M_rwdata_start, >> - PFN_DOWN(__pa(__2M_rwdata_start)), >> - PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start), >> - PAGE_HYPERVISOR_RW); >> + modify_xen_mappings((unsigned long)&__2M_rwdata_start, >> + (unsigned long)&__2M_rwdata_end, >> + _PAGE_NX | _PAGE_RW | _PAGE_PRESENT); > The only thing here is that you're changing from the centrally-defined > PAGE_HYPERVISOR_$TYPE flags to what might be called "magic > combinations" of flags. I realize this is because > modify_xen_mappings() only allows you to pass RW, PRESENT, and NX, but > it's still somewhat sub-optimal. > > Alternatives that come to mind: > 1. Mask out the other bits at the caller (i.e., PAGE_HYPERVISOR_RX & > MAP_PAGES_MASK) > 2. Have modify_xen_mappings() filter out the bits it doesn't want to change > 3. Extend modify_xen_mappings() to be able to modify other bits. > 4. Have modify_xen_mappings() assert that the only bits which are > *changing* are the FLAGS_MASK bits > > Thoughts? I suppose I could change the entry requirements and have "nf &= FLAGS_MASK" rather than the ASSERT()? That would allow the caller to pass the regular PAGE_HYPERVISOR_xxx, which I guess is what you mean by 2) 1) is a little nastly. 3) is hard. Most other bits you can't safely do blanket changes like this with. 4 is not possible). Over the range you might have different caching, different A/D bits or different global settings, which want to be kept as-are. ~Andrew
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index ca2d0bb..ccef946 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5956,7 +5956,17 @@ int populate_pt_range(unsigned long virt, unsigned long mfn, return map_pages_to_xen(virt, mfn, nr_mfns, MAP_SMALL_PAGES); } -void destroy_xen_mappings(unsigned long s, unsigned long e) +/* + * Alter the permissions of a range of Xen virtual address space. + * + * Does not create new mappings, and does not modify the mfn in existing + * mappings, but will shatter superpages if necessary, and will destroy + * mappings if not passed _PAGE_PRESENT. + * + * It is an error to call with present flags over an unpopulated range. It is + * an error to try to modify flags other than NX, RW and PRESENT. + */ +void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf) { bool_t locking = system_state > SYS_STATE_boot; l2_pgentry_t *pl2e; @@ -5964,8 +5974,12 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) unsigned int i; unsigned long v = s; + /* Set of valid PTE bits which may be altered. */ +#define FLAGS_MASK (_PAGE_NX|_PAGE_RW|_PAGE_PRESENT) + ASSERT(IS_ALIGNED(s, PAGE_SIZE)); ASSERT(IS_ALIGNED(e, PAGE_SIZE)); + ASSERT(!(nf & ~FLAGS_MASK)); while ( v < e ) { @@ -5973,6 +5987,9 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ) { + /* Confirm the caller isn't trying to create new mappings. */ + ASSERT(!(nf & _PAGE_PRESENT)); + v += 1UL << L3_PAGETABLE_SHIFT; v &= ~((1UL << L3_PAGETABLE_SHIFT) - 1); continue; @@ -5984,8 +6001,12 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) l1_table_offset(v) == 0 && ((e - v) >= (1UL << L3_PAGETABLE_SHIFT)) ) { - /* PAGE1GB: whole superpage is destroyed. */ - l3e_write_atomic(pl3e, l3e_empty()); + /* PAGE1GB: whole superpage is modified. */ + l3_pgentry_t nl3e = !(nf & _PAGE_PRESENT) ? l3e_empty() + : l3e_from_pfn(l3e_get_pfn(*pl3e), + (l3e_get_flags(*pl3e) & ~FLAGS_MASK) | nf); + + l3e_write_atomic(pl3e, nl3e); v += 1UL << L3_PAGETABLE_SHIFT; continue; } @@ -6016,6 +6037,9 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ) { + /* Confirm the caller isn't trying to create new mappings. */ + ASSERT(!(nf & _PAGE_PRESENT)); + v += 1UL << L2_PAGETABLE_SHIFT; v &= ~((1UL << L2_PAGETABLE_SHIFT) - 1); continue; @@ -6026,8 +6050,12 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) if ( (l1_table_offset(v) == 0) && ((e-v) >= (1UL << L2_PAGETABLE_SHIFT)) ) { - /* PSE: whole superpage is destroyed. */ - l2e_write_atomic(pl2e, l2e_empty()); + /* PSE: whole superpage is modified. */ + l2_pgentry_t nl2e = !(nf & _PAGE_PRESENT) ? l2e_empty() + : l2e_from_pfn(l2e_get_pfn(*pl2e), + (l2e_get_flags(*pl2e) & ~FLAGS_MASK) | nf); + + l2e_write_atomic(pl2e, nl2e); v += 1UL << L2_PAGETABLE_SHIFT; } else @@ -6055,13 +6083,23 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) } else { + l1_pgentry_t nl1e; + /* Ordinary 4kB mapping. */ pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(v); - l1e_write_atomic(pl1e, l1e_empty()); + + nl1e = !(nf & _PAGE_PRESENT) ? l1e_empty() + : l1e_from_pfn(l1e_get_pfn(*pl1e), + (l1e_get_flags(*pl1e) & ~FLAGS_MASK) | nf); + + l1e_write_atomic(pl1e, nl1e); v += PAGE_SIZE; - /* If we are done with the L2E, check if it is now empty. */ - if ( (v != e) && (l1_table_offset(v) != 0) ) + /* + * If we are destroying mappings and done with the L2E, check if + * it is now empty. + */ + if ( (nf & _PAGE_PRESENT) && (v != e) && (l1_table_offset(v) != 0) ) continue; pl1e = l2e_to_l1e(*pl2e); for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ ) @@ -6076,8 +6114,12 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) } } - /* If we are done with the L3E, check if it is now empty. */ - if ( (v != e) && (l2_table_offset(v) + l1_table_offset(v) != 0) ) + /* + * If we are destroying mappings and done with the L3E, check if it is + * now empty. + */ + if ( (nf & _PAGE_PRESENT) && (v != e) && + (l2_table_offset(v) + l1_table_offset(v) != 0) ) continue; pl2e = l3e_to_l2e(*pl3e); for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ ) @@ -6093,10 +6135,17 @@ void destroy_xen_mappings(unsigned long s, unsigned long e) } flush_area(NULL, FLUSH_TLB_GLOBAL); + +#undef FLAGS_MASK } #undef flush_area +void destroy_xen_mappings(unsigned long s, unsigned long e) +{ + modify_xen_mappings(s, e, 0); +} + void __set_fixmap( enum fixed_addresses idx, unsigned long mfn, unsigned long flags) { diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 22dc148..b867242 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1220,23 +1220,19 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( !using_2M_mapping() ) { /* Mark .text as RX (avoiding the first 2M superpage). */ - map_pages_to_xen(XEN_VIRT_START + MB(2), - PFN_DOWN(__pa(XEN_VIRT_START + MB(2))), - PFN_DOWN(__2M_text_end - - (const char *)(XEN_VIRT_START + MB(2))), - PAGE_HYPERVISOR_RX); + modify_xen_mappings(XEN_VIRT_START + MB(2), + (unsigned long)&__2M_text_end, + _PAGE_PRESENT); /* Mark .rodata as RO. */ - map_pages_to_xen((unsigned long)&__2M_rodata_start, - PFN_DOWN(__pa(__2M_rodata_start)), - PFN_DOWN(__2M_rodata_end - __2M_rodata_start), - PAGE_HYPERVISOR_RO); + modify_xen_mappings((unsigned long)&__2M_rodata_start, + (unsigned long)&__2M_rodata_end, + _PAGE_NX | _PAGE_PRESENT); /* Mark .data and .bss as RW. */ - map_pages_to_xen((unsigned long)&__2M_rwdata_start, - PFN_DOWN(__pa(__2M_rwdata_start)), - PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start), - PAGE_HYPERVISOR_RW); + modify_xen_mappings((unsigned long)&__2M_rwdata_start, + (unsigned long)&__2M_rwdata_end, + _PAGE_NX | _PAGE_RW | _PAGE_PRESENT); /* Drop the remaining mappings in the shattered superpage. */ destroy_xen_mappings((unsigned long)&__2M_rwdata_end, diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index d62394f..d4721fc 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -103,6 +103,8 @@ int map_pages_to_xen( unsigned long mfn, unsigned long nr_mfns, unsigned int flags); +/* Alter the permissions of a range of Xen virtual address space. */ +void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags); void destroy_xen_mappings(unsigned long v, unsigned long e); /* * Create only non-leaf page table entries for the