Re: [PATCH 1/3] KVM: arm64: vgic-its: Don't dereference a NULL collection on ITT save

[Fuad Tabba]

Hi Oliver,

On Thu, 6 Aug 2026 at 05:43, Oliver Upton <oupton@xxxxxxxxxx> wrote:
>
> Hi Fuad,
>
> Thanks for fixing this.
>
> On Wed, Aug 05, 2026 at 10:38:26AM +0100, Fuad Tabba wrote:
> > @@ -2119,6 +2119,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
> >       u32 next_offset;
> >       u64 val;
> >
> > +     /*
> > +      * MAPC with V=0 keeps the ITEs mapped but drops their collection,
> > +      * and with it the ICID. Save a zeroed entry, which the restore path
> > +      * reads back as invalid.
> > +      */
> > +     if (!ite->collection)
> > +             return vgic_its_write_entry_lock(its, gpa, 0ULL, ite);
> > +
>
> Writing a zero entry results in a suboptimal scan on the target,
> compute_next_eventid_offset() should just skip ITEs that are known
> invalid.

Agreed, I can do that on top on the respin (I still owe Sashiko one
for patch 3/3 [1]). The predecessor's next_offset still points at the
zeroed slot today, so the restore crawls from there to the next valid
ITE.

I don't think it replaces the write, though. Restore starts at event 0
and walks forward until it finds a valid entry, so an orphaned ITE at
or before that point is read directly, with no predecessor to skip it.
Leave the slot alone and an earlier save's entry is still sitting in
it: restore fails find_collection() and the migration dies with
-EINVAL on the destination. Zeroing is what DISCARD already does.

> Or better yet, we should just make the ITS emulation consistent
> with the migration behavior and delete ITEs when their corresponding
> collection is deleted. As you note, this is UNPRED after all :)

Happy to do that for MAPC(V=0), but I think it might not remove the
need for the check. vgic_its_free_collection() has a second caller:
with the ITS disabled, clearing GITS_BASER<coll>.Valid frees every
collection through vgic_its_free_collection_list() and NULLs
ite->collection, while the device list and the ITTs survive and
vgic_its_save_device_tables() only looks at the device BASER. I have a
selftest that oopses in vgic_its_save_ite() that way, with no MAPC in
it. And that one isn't UNPRED, so I'm not sure we can answer it by
deleting ITEs and zeroing their ITTs.

Cheers,
/fuad

[1] https://lore.kernel.org/all/CA+EHjTxuYLBwNsDnZd4B_jYWBDvGxXMWSDKPiMqjJjdCMYySaw@xxxxxxxxxxxxxx/

>
> Thanks,
> Oliver