exec: Skip mru section if it's a partial page and not resolving subpage
PATCH 1/1 exec: Skip mru section if it's a partial page and not resolving subpage
This fixes a crash caused by picking the wrong memory region in address_space_lookup_region seen with client code accessing a device model that uses alias memory regions.
--- exec.c | 1 + 1 file changed, 1 insertion(+)
exec.c +1 -0
@@ -413,6 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,bool update;if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED ] && + (resolve_subpage || !section->offset_within_region) &§ion_covers_addr (section, addr)) { update = false; }else {
Signature · 1 line
2.7.6
Quoted 15 lines · BALATON Zoltan
This fixes a crash caused by picking the wrong memory region in
address_space_lookup_regionseen with client code accessing a device model that uses alias memory regions.Signed-off-by BALATON Zoltan <[email protected]>--- exec.c | 1 + 1 file changed, 1 insertion(+)exec.c +0
-0
exec.c static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, · line 419
Quoted 7 lines
exec.c +1
-0@@ -413,6 +413,7 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,bool update;if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED ] && + (resolve_subpage || !section->offset_within_region) &§ion_covers_addr (section, addr)) { update = false; }else {
This is another possibility:
exec.c +3 -9
@@ -410,22 +410,16 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, {MemoryRegionSection *section =atomic_read (&d->mru_section);subpage_t *subpage; -bool update; -if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED ] && -section_covers_addr (section, addr)) { - update = false; - }else { +if (!section || section == &d->map.sections[PHYS_SECTION_UNASSIGNED ] || + !section_covers_addr (section, addr)) { section =phys_page_find (d, addr); - update = true; +atomic_set (&d->mru_section, section); }if (resolve_subpage && section->mr->subpage) { subpage =container_of (section->mr, subpage_t, iomem); section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX (addr)]]; } -if (update) { -atomic_set (&d->mru_section, section); - }return section; }
It will skip the expensive phys_page_find but not the cheap subpage lookup. Does it work for you?
Paolo
Quoted 33 lines · Paolo Bonzini
This is another possibility:
exec.c +3
-9@@ -410,22 +410,16 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, {MemoryRegionSection *section =atomic_read (&d->mru_section);subpage_t *subpage; -bool update; -if (section && section != &d->map.sections[PHYS_SECTION_UNASSIGNED ] && -section_covers_addr (section, addr)) { - update = false; - }else { +if (!section || section == &d->map.sections[PHYS_SECTION_UNASSIGNED ] || + !section_covers_addr (section, addr)) { section =phys_page_find (d, addr); - update = true; +atomic_set (&d->mru_section, section); }if (resolve_subpage && section->mr->subpage) { subpage =container_of (section->mr, subpage_t, iomem); section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX (addr)]]; } -if (update) { -atomic_set (&d->mru_section, section); - }return section; }
Quoted 2 lines
It will skip the expensive
phys_page_findbut not the cheap subpage lookup. Does it work for you?
This also works but I can't understand why becuase the problematic call to this function which got the wrong result from section_covers_addr was with resolve_subpage=false and in that case this looks identical to the original which did not work. Unless this function is called multiple times and another call with resolve_subpage=true now somehow works better with this change or replacing the mru_section earlier before subpage lookup makes a difference which is not obvious from this function.
Anyway, it fixes the problem I see so you can use this instead of my patch.
Thank you,
BALATON Zoltan
Quoted 38 lines · BALATON Zoltan
Quoted 37 lines · Paolo Bonzini
This is another possibility:
exec.c +0
-2@@ -410,22 +410,16 @@ static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d, { MemoryRegionSection *section = atomic_read(&d->mru_section); subpage_t *subpage; - bool update; - if (section && section !=&d->map.sections[PHYS_SECTION_UNASSIGNED] &&
-section_covers_addr(section, addr)) {
- update = false;
- } else {
+ if (!section || section == &d->map.sections[PHYS_SECTION_UNASSIGNED] ||
+ !section_covers_addr(section, addr)) { section =phys_page_find(d, addr);
- update = true;
+atomic_set(&d->mru_section, section);
}
if (resolve_subpage&§ion->mr->subpage) { subpage =container_of(section->mr, subpage_t, iomem); section = &d->map.sections[subpage->sub_section[SUBPAGE_IDX(addr)]]; }
- if (update) {
-atomic_set(&d->mru_section, section);
- }
return section;
}
Quoted 8 lines
Quoted 3 lines
It will skip the expensive
phys_page_findbut not the cheap subpage lookup.
Does it work for you?This also works but I can't understand why becuase the problematic call to this function which got the wrong result from
section_covers_addrwas with resolve_subpage=false and in that case this looks identical to the original which did not work.
The problem was that a resolve_subpage=true mru_section was reused for a resolve_subpage=false mru_section. This is fixed by always making the mru_section the same as if resolve_subpage=false (mru_section is set before looking into the subpage_t).
Thanks,
Paolo
Quoted 9 lines
Unless this function is called multiple
times and another call with resolve_subpage=true now somehow works better with this change or replacing themru_sectionearlier before subpage lookup makes a difference which is not obvious from this function.Anyway, it fixes the problem I see so you can use this instead of my patch.
Thank you,
BALATON Zoltan