| 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: <20171021143414.GA17884@cbox>
Date: Sat, 21 Oct 2017 16:34:14 +0200
From: Christoffer Dall <cdall@...aro.org>
To: Auger Eric <eric.auger@...hat.com>
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
On Sat, Oct 21, 2017 at 11:02:27AM +0200, Auger Eric wrote:
> 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()
>
>
> First my apologies as most of your comments have been left out of the
> v3-v4 respin by oversight. Some comments below.
> >
> > 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@...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?
>
> Handling deletion of device and collection separately is valid I think
> as MAPC (vgic_its_cmd_handle_mapc) and MAPD(vgic_its_cmd_handle_mapd)
> commands do that separately.
>
> However, ..., a collection can be referred by an ITE and I should reset
> the ite->collection = NULL for all ITEs referencing a deleted ITE.
> vgic_its_free_collection do that.
>
> By the way, vgic_its_unmap_device() is same as vgic_its_free_device() so
> I can remove vgic_its_free_device.
>
>
> >
> >> +}
> >> +
> >> +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 ?
> damned.
> >
> >> +}
> >> +
> >> +
> >> 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.
>
> I think you agreed on my previous statement:
> https://www.spinics.net/lists/kvm-arm/msg27198.html
>
>
> I understand the sequence is:
> 1) vm_ioctl_create_device
> |_ ops->create
> |_ vgic_create_its
> INIT_LIST_HEAD(&its->device_list);
> INIT_LIST_HEAD(&its->collection_list);
> list_add(&dev->vm_node, &kvm->devices);
>
> kvm_destroy_devices
> list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
> ops->destroy
> |_ vgic_its_destroy
>
> so vgic_its_destroy is called on an its device that was added to the
> kvm->devices list. If so the list was created.
>
> Then we have vgic_mmio_write_its_baser() which is new caller introduced
> in subsequent patch.
>
> for vgic_mmio_write_its_baser() to be called, vgic_register_its_iodev
> must have been called. This latter is called on set_attr=vgic_its_set_attr
> set_attr can be called only if the fd is created. This happens in
> kvm_ioctl_create_device after ops->create() has been successful, ie
> meaning the lists are created.
>
> What do I miss? What is the case you identified where the device_list is
> not initialized?
>
I am probably just remembering incorrect, I just thought we identified
some strange flow where this could happen, but I can't do that anymore,
so I'll stop asking this question. Sorry about that.
-Christoffer
Powered by blists - more mailing lists