[PATCH] KVM: arm/arm64: vgic-its: Add error handling in vgic_its_cache_translation

[Keisuke Nishimura]

On 29/11/2024 08:20, Oliver Upton wrote:
> On Thu, Nov 28, 2024 at 08:04:01PM +0100, Keisuke Nishimura wrote:
>> Hello Marc and Oliver,
>>
>> Thank you for the replies.
>>
>> On 28/11/2024 18:55, Oliver Upton wrote:
>>> On Thu, Nov 28, 2024 at 02:45:34PM +0100, Keisuke Nishimura wrote:
>>>> The xa_store() may fail because there is no guarantee that the cache_key
>>>> index is already used in its->translation_cache. This fix (1) resolves
>>>> the kref inconsistency on failure and (2) returns the error code.
>>>
>>> xa_store() doesn't fail if an entry is already present at the specified
>>> index. It returns the old entry, which is why we have a vgic_put_irq()
>>> on the "error" path.
>>
>> Sure. But to my understanding, this could be the first store to
>> its->translation_cache with cache_key, and that is why old can be NULL? I am
>> not very familiar with this code, so please correct me if I am wrong.
> 
> So we take a reference on @irq to represent the pointer to it that's
> stored in the translation cache. There are 3 possible outcomes after the
> store:
> 
>   - Store succeeds and no pre-existing entry.
> 
>   - Store succeeds and there's a pre-existing entry. put the reference on
>     @old since it was evicted from the translation cache
> 
>   - Store fails because of a failed memory allocation / error in xarray
>     library.
> 
> We don't handle #3, and the correct thing to do in this case is to put
> @irq since it was never made visible in the translation cache.
> 
> So I think we'd want to do something similar to the following.
> 

Hello,

Thanks for the comment. xa_reserve() can allocate the memory in advance and 
ensure the success of the subsequent xa_store(). FYI, the document of 
xa_reserve() says:

 > Ensures there is somewhere to store an entry at index in the array. If there 
is already something stored at index, this function does nothing. If there was 
nothing there, the entry is marked as reserved. Loading from a reserved entry 
returns a NULL pointer.


I am thinking of calling it before the reference in v2. Here is the suggestion:

---
  arch/arm64/kvm/vgic/vgic-its.c | 9 +++++++++
  1 file changed, 9 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 198296933e7e..1422812ad6d7 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -561,11 +561,20 @@ static void vgic_its_cache_translation(struct kvm *kvm, 
struct vgic_its *its,
  {
  	unsigned long cache_key = vgic_its_cache_key(devid, eventid);
  	struct vgic_irq *old;
+	int ret;

  	/* Do not cache a directly injected interrupt */
  	if (irq->hw)
  		return;

+	/*
+	 * Ensure the following xa_store() will not fail. Intentionally do
+	 * not return the error as the translation cache is best effort.
+	 */
+	ret = xa_reserve(&its->translation_cache, cache_key, GFP_KERNEL_ACCOUNT);
+	if (ret)
+		return;
+
  	/*
  	 * The irq refcount is guaranteed to be nonzero while holding the
  	 * its_lock, as the ITE (and the reference it holds) cannot be freed.
-- 
2.34.1