| Message ID | 20200120143142.19820-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/x86: domain: Free all the pages associated to struct domain | expand |
On 20.01.2020 15:31, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The structure domain may be bigger than a page size when lock profiling > is enabled. However, the function free_domheap_struct will only free the > first page. > > This is not a security issue because struct domain can only be bigger > than a page size for lock profiling. The feature can only be selected > in DEBUG and EXPERT mode. > > Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled") > Reported-by: David Woodhouse <dwmw@amazon.co.uk> > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On Mon, Jan 20, 2020 at 02:31:42PM +0000, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The structure domain may be bigger than a page size when lock profiling > is enabled. However, the function free_domheap_struct will only free the > first page. > > This is not a security issue because struct domain can only be bigger > than a page size for lock profiling. The feature can only be selected > in DEBUG and EXPERT mode. > > Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled") > Reported-by: David Woodhouse <dwmw@amazon.co.uk> > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 20/01/2020 14:31, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The structure domain may be bigger than a page size when lock profiling > is enabled. However, the function free_domheap_struct will only free the > first page. > > This is not a security issue because struct domain can only be bigger > than a page size for lock profiling. The feature can only be selected > in DEBUG and EXPERT mode. > > Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled") > Reported-by: David Woodhouse <dwmw@amazon.co.uk> > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/x86/domain.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 28fefa1f81..a5380b9bab 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void) > > void free_domain_struct(struct domain *d) > { > - free_xenheap_page(d); > + free_xenheap_pages(d, get_order_from_bytes(sizeof(*d))); :( I'm entirely certain I raised this during the review of the original patch. I'd much rather see the original patch reverted. The current size of struct domain with lockprofile enabled is 3200 bytes. ~Andrew
Hi Andrew, On 22/01/2020 12:52, Andrew Cooper wrote: > On 20/01/2020 14:31, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> The structure domain may be bigger than a page size when lock profiling >> is enabled. However, the function free_domheap_struct will only free the >> first page. >> >> This is not a security issue because struct domain can only be bigger >> than a page size for lock profiling. The feature can only be selected >> in DEBUG and EXPERT mode. >> >> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled") >> Reported-by: David Woodhouse <dwmw@amazon.co.uk> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> --- >> xen/arch/x86/domain.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c >> index 28fefa1f81..a5380b9bab 100644 >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void) >> >> void free_domain_struct(struct domain *d) >> { >> - free_xenheap_page(d); >> + free_xenheap_pages(d, get_order_from_bytes(sizeof(*d))); > > :( > > I'm entirely certain I raised this during the review of the original patch. > > I'd much rather see the original patch reverted. The current size of > struct domain with lockprofile enabled is 3200 bytes. Let me have a look first to see when/why struct domain is less than 4K with lockprofile. Cheers,
On 22/01/2020 13:13, Julien Grall wrote: > Hi Andrew, > > On 22/01/2020 12:52, Andrew Cooper wrote: >> On 20/01/2020 14:31, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> The structure domain may be bigger than a page size when lock profiling >>> is enabled. However, the function free_domheap_struct will only free >>> the >>> first page. >>> >>> This is not a security issue because struct domain can only be bigger >>> than a page size for lock profiling. The feature can only be selected >>> in DEBUG and EXPERT mode. >>> >>> Fixes: 8916fcf4577 ("x86/domain: compile with lock_profile=y enabled") >>> Reported-by: David Woodhouse <dwmw@amazon.co.uk> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> --- >>> xen/arch/x86/domain.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c >>> index 28fefa1f81..a5380b9bab 100644 >>> --- a/xen/arch/x86/domain.c >>> +++ b/xen/arch/x86/domain.c >>> @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void) >>> void free_domain_struct(struct domain *d) >>> { >>> - free_xenheap_page(d); >>> + free_xenheap_pages(d, get_order_from_bytes(sizeof(*d))); >> >> :( >> >> I'm entirely certain I raised this during the review of the original >> patch. >> >> I'd much rather see the original patch reverted. The current size of >> struct domain with lockprofile enabled is 3200 bytes. > > Let me have a look first to see when/why struct domain is less than 4K > with lockprofile. In the intervening time, Juergen has totally rewritten lock profiling, and the virtual timer infrastructure has been taken out-of-line. Its probably the latter which is the dominating factor. ~Andrew
On Wed, 2020-01-22 at 13:15 +0000, Andrew Cooper wrote: > > > I'd much rather see the original patch reverted. The current size of > > > struct domain with lockprofile enabled is 3200 bytes. > > > > Let me have a look first to see when/why struct domain is less than 4K > > with lockprofile. > > In the intervening time, Juergen has totally rewritten lock profiling, > and the virtual timer infrastructure has been taken out-of-line. > > Its probably the latter which is the dominating factor. OK, so if we revert 8916fcf4577 is it reasonable for me to then assume that 'struct domain' will always fit within a page, and declare that live update shall not work to a Xen where that isn't true? I have a nasty trick in mind... During live update, we need to do a pass over the live update data in early boot in order to work out which pages to reserve. That part has to be done early, while the boot allocator is active. It works by setting the PGC_allocated bit in the page_info of the reserved pages, so that init_heap_pages() knows not to include them. I've posted that part already. Also during live update, we need to consume the actual domain state that was passed over from the previous Xen, and fill in the owner (and refcount etc.) in the same page_info structures, before those pages are in a truly consistent state. Right now, we need the latter to happen *after* the boot allocator is done and we're able to allocate from the heap... because we need to be able to allocate the domain structures, and we don't want to ensure that there's enough space in the LU reserved bootmem for that many domain structures. Hence the nasty trick: What if we allocate the new struct domain on *top* of the old one, since we happen to know that page *wasn't* used by the previous Xen for anything that needs to be preserved. The structure itself isn't an ABI and never can be, and it will have to be repopulated from the live migration data, of course — but if we can at least make the assumption that it'll *fit*, then perhaps we can manage to do both of the above with only one pass over all the domain pages. This will have a direct effect on the customer-observed pause time for a live update. So it's kind of icky, but also *very* tempting... Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Hi David, On 22/01/2020 13:50, Woodhouse, David wrote: > On Wed, 2020-01-22 at 13:15 +0000, Andrew Cooper wrote: >>>> I'd much rather see the original patch reverted. The current size of >>>> struct domain with lockprofile enabled is 3200 bytes. >>> >>> Let me have a look first to see when/why struct domain is less than 4K >>> with lockprofile. >> >> In the intervening time, Juergen has totally rewritten lock profiling, >> and the virtual timer infrastructure has been taken out-of-line. >> >> Its probably the latter which is the dominating factor. > > OK, so if we revert 8916fcf4577 is it reasonable for me to then assume > that 'struct domain' will always fit within a page, and declare that > live update shall not work to a Xen where that isn't true? While it is nice to have struct domain small, I would rather not bake to size assumption in live update. The more on Arm we have been quite often close to the limit. So I don't want to limit my choices. > > I have a nasty trick in mind... > > During live update, we need to do a pass over the live update data in > early boot in order to work out which pages to reserve. That part has > to be done early, while the boot allocator is active. It works by > setting the PGC_allocated bit in the page_info of the reserved pages, > so that init_heap_pages() knows not to include them. I've posted that > part already. > > Also during live update, we need to consume the actual domain state > that was passed over from the previous Xen, and fill in the owner (and > refcount etc.) in the same page_info structures, before those pages are > in a truly consistent state. > > Right now, we need the latter to happen *after* the boot allocator is > done and we're able to allocate from the heap... because we need to be > able to allocate the domain structures, and we don't want to ensure > that there's enough space in the LU reserved bootmem for that many > domain structures. As you pointed out above, the struct domain itself is a page (i.e 4KB on x86). Let say you have 256 domains, this would only use 1MB. Which is not too bad to take into account. But struct domain has a lot of out-of-line allocation. If you are not planning to make struct domain part of the ABI, then you would need quite a few allocations even in your cases. Of course, you could just half initialize struct domain. However, I would be cautious with this solution as it would be more difficult to chase down bug around struct page. Imagine the domain pointer is accessed earlier than expected. > Hence the nasty trick: What if we allocate the new struct domain on > *top* of the old one, since we happen to know that page *wasn't* used > by the previous Xen for anything that needs to be preserved. The > structure itself isn't an ABI and never can be, and it will have to be > repopulated from the live migration data, of course — but if we can at > least make the assumption that it'll *fit*, then perhaps we can manage > to do both of the above with only one pass over all the domain pages. > > This will have a direct effect on the customer-observed pause time for > a live update. So it's kind of icky, but also *very* tempting... May I recommend to get some numbers with the multiple pass and "nicer" code first? We can decide on that what sort of hackery we need to lower the pause time. Cheers,
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 28fefa1f81..a5380b9bab 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -344,7 +344,7 @@ struct domain *alloc_domain_struct(void) void free_domain_struct(struct domain *d) { - free_xenheap_page(d); + free_xenheap_pages(d, get_order_from_bytes(sizeof(*d))); } struct vcpu *alloc_vcpu_struct(const struct domain *d)