Re: [PATCH v9 13/27] gunyah: vm_mgr: Add/remove user memory regions

Elliot Berman
2023-01-30T21:46:51Z

On 1/25/2023 5:34 AM, Srivatsa Vaddagiri wrote:
> * Elliot Berman <quic_eberman@quicinc.com> [2023-01-20 14:46:12]:
>
>> + /* Check for overlap */
>> + list_for_each_entry(tmp_mapping, &ghvm->memory_mappings, list) {
>> + if (!((mapping->guest_phys_addr + (mapping->npages << PAGE_SHIFT) <=
>> + tmp_mapping->guest_phys_addr) ||
>> + (mapping->guest_phys_addr >=
>> + tmp_mapping->guest_phys_addr + (tmp_mapping->npages << PAGE_SHIFT)))) {
>> + ret = -EEXIST;
>> + goto unlock;
>> + }
>> + }
>> +
>> + list_add(&mapping->list, &ghvm->memory_mappings);
>
> I think the potential race condition described last time is still possible. Pls
> check.
>
>> +unlock:
>> + mutex_unlock(&ghvm->mm_lock);
>> + if (ret)
>> + goto free_mapping;
>> +
>> + mapping->pages = kcalloc(mapping->npages, sizeof(*mapping->pages), GFP_KERNEL);
>> + if (!mapping->pages) {
>> + ret = -ENOMEM;
>> + goto reclaim;
>
> Same comment as last time. Can you check this error path? We seem to call
> unpin_user_page() in this path, which is not correct.
>
>> + }
>> +
>> + pinned = pin_user_pages_fast(region->userspace_addr, mapping->npages,
>> + FOLL_WRITE | FOLL_LONGTERM, mapping->pages);
>> + if (pinned < 0) {
>> + ret = pinned;
>> + goto reclaim;
>
> Same comment as above.
>

Caught the race condition you are describing between adding and removing
the memory parcels. Should be fixed for v10.

- Elliot