linux-kernel - Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two buses

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150821112946.64ae6f7b.cornelia.huck@de.ibm.com>
Date:	Fri, 21 Aug 2015 11:29:46 +0200
From:	Cornelia Huck <cornelia.huck@...ibm.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	gleb@...nel.org, pbonzini@...hat.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>
Subject: Re: [PATCH 2/3] kvm: don't register wildcard MMIO EVENTFD on two
 buses

On Fri, 21 Aug 2015 16:03:52 +0800
Jason Wang <jasowang@...hat.com> wrote:


> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 9ff4193..834a409 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -838,11 +838,6 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  
>  	kvm_iodevice_init(&p->dev, &ioeventfd_ops);
>  
> -	ret = kvm_io_bus_register_dev(kvm, bus_idx, p->addr, p->length,
> -				      &p->dev);
> -	if (ret < 0)
> -		goto unlock_fail;
> -
>  	/* When length is ignored, MMIO is also put on a separate bus, for
>  	 * faster lookups.

You probably want to change this comment as well?

>  	 */
> @@ -850,9 +845,15 @@ kvm_assign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)

Unfortunately snipped by diff, but the check here is on !len && !PIO,
which only does the desired thing as VIRTIO_CCW always uses len == 8.
Should the check be for !len && MMIO instead?

>  		ret = kvm_io_bus_register_dev(kvm, KVM_FAST_MMIO_BUS,
>  					      p->addr, 0, &p->dev);
>  		if (ret < 0)
> -			goto register_fail;
> +			goto unlock_fail;
> +	} else {
> +		ret = kvm_io_bus_register_dev(kvm, bus_idx, p->addr, p->length,
> +					      &p->dev);
> +		if (ret < 0)
> +			goto unlock_fail;
>  	}

Hm... maybe the following would be more obvious:

my_bus = (p->length == 0) && (bus_idx == KVM_MMIO_BUS) ? KVM_FAST_MMIO_BUS : bus_idx;
ret = kvm_io_bus_register_dev(kvm, my_bus, p->addr, p->length, &pdev->dev); 

>  
> +
>  	kvm->buses[bus_idx]->ioeventfd_count++;
>  	list_add_tail(&p->list, &kvm->ioeventfds);

(...)

> @@ -900,10 +899,11 @@ kvm_deassign_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  		if (!p->wildcard && p->datamatch != args->datamatch)
>  			continue;
>  
> -		kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
>  		if (!p->length) {
>  			kvm_io_bus_unregister_dev(kvm, KVM_FAST_MMIO_BUS,
>  						  &p->dev);
> +		} else {
> +			kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
>  		}

Similar comments here... do you want to check for bus_idx ==
KVM_MMIO_BUS as well?

>  		kvm->buses[bus_idx]->ioeventfd_count--;
>  		ioeventfd_release(p);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/