| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <69d05995-8e1e-f1da-795c-94bf1bb8a16e@redhat.com>
Date: Fri, 13 Oct 2017 16:37:31 +0200
From: Auger Eric <eric.auger@...hat.com>
To: Christoffer Dall <cdall@...aro.org>
Cc: eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, marc.zyngier@....com,
peter.maydell@...aro.org, andre.przywara@....com,
wanghaibin.wang@...wei.com, wu.wubin@...wei.com
Subject: Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions
to free the caches
Hi Christoffer,
On 13/10/2017 15:35, Christoffer Dall wrote:
> On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote:
>> From: wanghaibin <wanghaibin.wang@...wei.com>
>>
>> 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?
This paragraph was an attempt to explain why we could remove the above
check but it looks I need to rephrase ;-)
Thanks
Eric
>
>>
>> We also move vgic_its_free_device to prepare for new callers.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@...wei.com>
>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>>
>> ---
>> [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
>
Powered by blists - more mailing lists