Re: [PATCH v3 1/4] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

[Ackerley Tng]

"David Hildenbrand (Arm)" <david@xxxxxxxxxx> writes:

>
> [...snip...]
>
>> -	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
>> +	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
>> +
>> +	r = xa_is_err(xar) ? xa_err(xar) : 0;
>
>
> r = xa_err(xar);
>
> Should be sufficient, right?
>
> mm/memremap.c:pagemap_range() uses that and just avoids the intermediate xar
> value completely.
>
> r = xa_err(xa_store_range(...);
>
>> +	if (r) {
>> +		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
>> +		slot->gmem.file = NULL;
>> +		slot->gmem.pgoff = 0;
>> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;

Was wondering if the changelog should explain why not move
xa_store_range() before setting up these 3 fields that need undoing.

IIUC the reason is that other parts of gmem code expect any slots in
bindings to have a non-NULL gmem.file?

>> +	}
>>  	filemap_invalidate_unlock(inode->i_mapping);
>
> Apart from that
>
> Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>

+1 on using xa_err() directly.

Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>

> --
> Cheers,
>
> David