[PATCH] KVM: arm/arm64: vgic-its: Add error handling in vgic_its_cache_translation
Oliver Upton
oliver.upton at linux.dev
Thu Nov 28 23:20:59 PST 2024
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.
--
Thanks,
Oliver
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index f4c4494645c3..fb96802799c6 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -608,12 +608,22 @@ static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
lockdep_assert_held(&its->its_lock);
vgic_get_irq_kref(irq);
+ old = xa_store(&its->translation_cache, cache_key, irq, GFP_KERNEL_ACCOUNT);
+
+ /*
+ * Put the reference taken on @irq if the store fails. Intentionally do
+ * not return the error as the translation cache is best effort.
+ */
+ if (xa_is_err(old)) {
+ vgic_put_irq(kvm, irq);
+ return;
+ }
+
/*
* We could have raced with another CPU caching the same
* translation behind our back, ensure we don't leak a
* reference if that is the case.
*/
- old = xa_store(&its->translation_cache, cache_key, irq, GFP_KERNEL_ACCOUNT);
if (old)
vgic_put_irq(kvm, old);
}
More information about the linux-arm-kernel
mailing list