Re: [Xen-devel] vfree crash

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/07/2019 11:47, Jan Beulich wrote:
> On 01.07.2019 10:56, Andrew Cooper wrote:
>> On 01/07/2019 09:45, Petre Ovidiu PIRCALABU wrote:
>>> The problem lies with vfree because it creates a new list with the
>>> pages, unmaps the va pointer and then frees the pages. If I do these
>>> steps manually (without adding them to a new list) it works.
>> The problem here is that struct page_info only has a single linked list
>> pointer, and vfree() blindly assumes it is available for use, which
>> isn't true once you've called assign_pages() on the vmap area.
>>
>> At the moment, it doesn't look like it is possible to set v*alloc()'d
>> pages up suitably to be mapped by a guest.  (Similar corruption will
>> occur via share_xen_page_with_guest() and the xenheap list).
> Well, whoever assigns pages to a domain behind vmalloc()'s back has got
> to make sure to de-assign those pages before freeing them.

Why?  Or perhaps more accurately, where is any of this written down.

Allocation of memory seems logically unrelated to making it mappable by
guests, so when vmalloc() *is* the correct allocation function to use,
the fact that assign_pages() results in vfree() silently corrupting the
domains memory list is unexpected behaviour.

> An alternative
> _might_ be to leave freeing to the normal cleanup processes (when the
> last page ref gets put), and just vunmap()-ing the range when the mapping
> isn't needed anymore.

So this is what I suggested as an interim solution, but I'm not sure if
it is a sensible option longterm.

The scenario here is for the "vm-event-ng" interface which was posted as
an RFC earlier.  Several key purposes for the new interface is to be a
slot-per-vcpu, and to be usable via the acquire_resource infrastructure.

struct vm_event_st is currently 384 bytes, which is only 10 full structs
per page.  The size of the structure is liable to change over time, and
most likely won't evenly divide a page, so vmalloc() is the correct
allocation interface within Xen.

The alloc and free in this case is being done as a side effect of the
vmi enable/disable calls.  The lifetime of the VMI interface isn't the
same as the lifetime of the VM.

With HVI specifically, the SVA VM can reboot, and it needs to re-attach
to the protected VMs.  There are other load balancing scenarios where
the protection of a VM might logically move between two different SVAs.

In either case, retaining the first vmalloc() will result in a failure
to remap the ring, as the domain assignment will be to the old domid.

Therefore, I think it is important to be able to fully disable and clean
up the VMI interface at some point before the protected VM is destroyed.

As a result, I think the proper fix here is to modify vfree() not to
clobber the pagelist.

Thoughts?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel