On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote:
> From: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
>
> We create 2 new functions that frees the device and
two free
> collection lists. this is currently called by vgic_its_destroy()
These are
> and we will add other callers in subsequent patches.
>
> We also remove the check on its->device_list.next as it looks
> unnecessary:
Could you elude to why you're doing this in the first place in the next
version of the commit message? Thanks.
>
> The kvm device is removed by kvm_destroy_devices which loops on
> all the devices added to kvm->devices. kvm_ioctl_create_device
> only adds the device to kvm_devices once the lists have been
> initialized (in vgic_create_its).
I don't understand what this paragraph is trying to tell me beyond what
some code already does irrelevant to this patch?
>
> We also move vgic_its_free_device to prepare for new callers.
>
> Signed-off-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>
> ---
> [Eric] removed its->device_list.next which is not needed as
> pointed out by Wanghaibin. Reword the commit message
> ---
> virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
> 1 file changed, 41 insertions(+), 35 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 9e6b556..0df6d5f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
> kfree(ite);
> }
>
> +static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> +{
> + struct its_ite *ite, *tmp;
> +
> + list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> + its_free_ite(kvm, ite);
> + list_del(&dev->dev_list);
> + kfree(dev);
> +}
> +
> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
> +{
> + struct list_head *cur, *temp;
> +
> + mutex_lock(&its->its_lock);
> + list_for_each_safe(cur, temp, &its->device_list) {
> + struct its_device *dev;
> +
> + dev = list_entry(cur, struct its_device, dev_list);
> + vgic_its_free_device(kvm, dev);
> + }
> + mutex_unlock(&its->its_lock);
this changes semantics from locking across freeing both devices and
collections to taking the locks separately. Is that valid?
> +}
> +
> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
> +{
> + struct list_head *cur, *temp;
> +
> + list_for_each_safe(cur, temp, &its->collection_list) {
> + struct its_collection *coll;
> +
> + coll = list_entry(cur, struct its_collection, coll_list);
> + list_del(cur);
> + kfree(coll);
> + }
> + mutex_unlock(&its->its_lock);
no mutex_lock ?
> +}
> +
> +
> static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
> {
> return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
> @@ -1634,46 +1673,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
> return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
> }
>
> -static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> -{
> - struct its_ite *ite, *tmp;
> -
> - list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
> - its_free_ite(kvm, ite);
> - list_del(&dev->dev_list);
> - kfree(dev);
> -}
> -
> static void vgic_its_destroy(struct kvm_device *kvm_dev)
> {
> struct kvm *kvm = kvm_dev->kvm;
> struct vgic_its *its = kvm_dev->private;
> - struct list_head *cur, *temp;
> -
> - /*
> - * We may end up here without the lists ever having been initialized.
> - * Check this and bail out early to avoid dereferencing a NULL pointer.
> - */
> - if (!its->device_list.next)
> - return;
I don't think this is valid. We can actually have a non-initialized
list and without this check, list_for_each_entry_safe in
vgic_its_free_device_list will crash the kernel.
Note that an initialized empty list_head doesn't have head and tail
pointing to NULL, but pointing to the list_head itself.
> -
> - mutex_lock(&its->its_lock);
> - list_for_each_safe(cur, temp, &its->device_list) {
> - struct its_device *dev;
> -
> - dev = list_entry(cur, struct its_device, dev_list);
> - vgic_its_free_device(kvm, dev);
> - }
> -
> - list_for_each_safe(cur, temp, &its->collection_list) {
> - struct its_collection *coll;
> -
> - coll = list_entry(cur, struct its_collection, coll_list);
> - list_del(cur);
> - kfree(coll);
> - }
> - mutex_unlock(&its->its_lock);
>
> + vgic_its_free_device_list(kvm, its);
> + vgic_its_free_collection_list(kvm, its);
> kfree(its);
> }
>
> --
> 2.5.5
>
Thanks,
-Christoffer
