On Tue, May 03, 2022 at 07:40:46PM +0200, Eric Auger wrote:
> Hi Ricardo,
>
> On 4/27/22 20:48, Ricardo Koller wrote:
> > Restoring a corrupted collection entry is being ignored and treated as
> maybe precise what is a corrupted ITE (out of range id or not matching
> guest RAM)
> > success. More specifically, vgic_its_restore_cte failure is treated as
> > success by vgic_its_restore_collection_table. vgic_its_restore_cte uses
> > a positive number to return ITS error codes, and +1 to return success.
> Not fully correct as vgic_its_restore_cte() also returns a bunch of
> generic negative error codes. vgic_its_alloc_collection() only returns
> one positive ITS error code.
Thanks, will clarify this. I was just focusing on that positive ITS
error code being treated as success by the caller.
> > The caller then uses "ret > 0" to check for success. An additional issue
> > is that invalid entries return 0 and although that doesn't fail the
> > restore, it leads to skipping all the next entries.
> Isn't what we want. If I remember correctly an invalid entry corresponds
> to the end of the collection table, hence the break.
> see vgic_its_save_collection_table() and "add a last dummy element with
> valid bit unset".
Ah, definitely. This was incorrect then.
> >
> > Fix this by having vgic_its_restore_cte return negative numbers on
> > error, and 0 on success (which includes skipping an invalid entry).
> > While doing that, also fix alloc_collection return codes to not mix ITS
> > error codes (positive numbers) and generic error codes (negative
> > numbers).
> >
> > Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++----------
> > 1 file changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index fb2d26a73880..86c26aaa8275 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -999,15 +999,16 @@ static bool vgic_its_check_event_id(struct vgic_its *its, struct its_device *dev
> > return __is_visible_gfn_locked(its, gpa);
> > }
> >
> > +/*
> > + * Adds a new collection into the ITS collection table.
> nit: s/Adds/Add here and below
> > + * Returns 0 on success, and a negative error value for generic errors.
> > + */
> > static int vgic_its_alloc_collection(struct vgic_its *its,
> > struct its_collection **colp,
> > u32 coll_id)
> > {
> > struct its_collection *collection;
> >
> > - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> > - return E_ITS_MAPC_COLLECTION_OOR;
> > -
> > collection = kzalloc(sizeof(*collection), GFP_KERNEL_ACCOUNT);
> > if (!collection)
> > return -ENOMEM;
> > @@ -1101,7 +1102,12 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >
> > collection = find_collection(its, coll_id);
> > if (!collection) {
> > - int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > + int ret;
> > +
> > + if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> > + return E_ITS_MAPC_COLLECTION_OOR;
> > +
> > + ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > if (ret)
> > return ret;
> > new_coll = collection;
> > @@ -1256,6 +1262,10 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
> > if (!collection) {
> > int ret;
> >
> > + if (!vgic_its_check_id(its, its->baser_coll_table,
> > + coll_id, NULL))
> > + return E_ITS_MAPC_COLLECTION_OOR;
> > +
> > ret = vgic_its_alloc_collection(its, &collection,
> > coll_id);
> > if (ret)
> > @@ -2497,6 +2507,10 @@ static int vgic_its_save_cte(struct vgic_its *its,
> > return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
> > }
> >
> > +/*
> > + * Restores a collection entry into the ITS collection table.
> > + * Returns 0 on success, and a negative error value for generic errors.
> > + */
> > static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> > {
> > struct its_collection *collection;
> > @@ -2511,7 +2525,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> > return ret;
> > val = le64_to_cpu(val);
> > if (!(val & KVM_ITS_CTE_VALID_MASK))
> > - return 0;
> > + return 0; /* invalid entry, skip it */
> >
> > target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
> > coll_id = val & KVM_ITS_CTE_ICID_MASK;
> > @@ -2523,11 +2537,15 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> > collection = find_collection(its, coll_id);
> > if (collection)
> > return -EEXIST;
> > +
> > + if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> > + return -EINVAL;
> > +
> > ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > if (ret)
> > return ret;
> > collection->target_addr = target_addr;
> > - return 1;
> > + return 0;
> > }
> >
> > /**
> > @@ -2593,15 +2611,12 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
> >
> > while (read < max_size) {
> > ret = vgic_its_restore_cte(its, gpa, cte_esz);
> > - if (ret <= 0)
> > + if (ret < 0)
> > break;
> > gpa += cte_esz;
> > read += cte_esz;
> > }
> >
> > - if (ret > 0)
> > - return 0;
> > -
> > return ret;
> > }
> >
> Thanks
>
> Eric
>
Thanks,
Ricardo
