On 07/07/2017 12:58, Christian Borntraeger wrote:
> mark kvm->busses as rcu protected and use the correct access
> function everywhere.
>
> found by sparse
> virt/kvm/kvm_main.c:3490:15: error: incompatible types in comparison expression (different address spaces)
> virt/kvm/kvm_main.c:3509:15: error: incompatible types in comparison expression (different address spaces)
> virt/kvm/kvm_main.c:3561:15: error: incompatible types in comparison expression (different address spaces)
> virt/kvm/kvm_main.c:3644:15: error: incompatible types in comparison expression (different address spaces)
>
> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> ---
> include/linux/kvm_host.h | 2 +-
> virt/kvm/eventfd.c | 8 +++++---
> virt/kvm/kvm_main.c | 17 ++++++++++-------
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7c32905..220f251 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -404,7 +404,7 @@ struct kvm {
> int last_boosted_vcpu;
> struct list_head vm_list;
> struct mutex lock;
> - struct kvm_io_bus *buses[KVM_NR_BUSES];
> + struct kvm_io_bus __rcu *buses[KVM_NR_BUSES];
> #ifdef CONFIG_HAVE_KVM_EVENTFD
> struct {
> spinlock_t lock;
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index a8d5403..3e08b43 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -825,7 +825,7 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
> if (ret < 0)
> goto unlock_fail;
>
> - kvm->buses[bus_idx]->ioeventfd_count++;
> + rcu_dereference_protected(kvm->buses[bus_idx], 1)->ioeventfd_count++;
Whenever possible, we should use (s)rcu_dereference_check and a better
predicate than "1". In this case:
static inline struct kvm_io_bus *kvm_get_bus(struct kvm *kvm, int i)
{
struct kvm_io_bus **buses =
srcu_dereference_check(kvm->buses, &kvm->srcu,
lockdep_is_held(&kvm->slots_lock));
return buses[i];
}
(since you are at it, __kvm_memslots should also use srcu_dereference_check
instead of rcu_dereference_check!).
Paolo
> list_add_tail(&p->list, &kvm->ioeventfds);
>
> mutex_unlock(&kvm->slots_lock);
> @@ -848,6 +848,7 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
> {
> struct _ioeventfd *p, *tmp;
> struct eventfd_ctx *eventfd;
> + struct kvm_io_bus *bus;
> int ret = -ENOENT;
>
> eventfd = eventfd_ctx_fdget(args->fd);
> @@ -870,8 +871,9 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx,
> continue;
>
> kvm_io_bus_unregister_dev(kvm, bus_idx, &p->dev);
> - if (kvm->buses[bus_idx])
> - kvm->buses[bus_idx]->ioeventfd_count--;
> + bus = rcu_dereference_protected(kvm->buses[bus_idx], 1);
> + if (bus)
> + bus->ioeventfd_count--;
> ioeventfd_release(p);
> ret = 0;
> break;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e3e6fec..e5ae3eb 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -679,8 +679,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
> if (init_srcu_struct(&kvm->irq_srcu))
> goto out_err_no_irq_srcu;
> for (i = 0; i < KVM_NR_BUSES; i++) {
> - kvm->buses[i] = kzalloc(sizeof(struct kvm_io_bus),
> - GFP_KERNEL);
> + rcu_assign_pointer(kvm->buses[i],
> + kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL));
> if (!kvm->buses[i])
> goto out_err;
> }
> @@ -705,7 +705,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> hardware_disable_all();
> out_err_no_disable:
> for (i = 0; i < KVM_NR_BUSES; i++)
> - kfree(kvm->buses[i]);
> + kfree(rcu_access_pointer(kvm->buses[i]));
> for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
> kvm_free_memslots(kvm,
> rcu_dereference_protected(kvm->memslots[i], 1));
> @@ -741,8 +741,11 @@ static void kvm_destroy_vm(struct kvm *kvm)
> spin_unlock(&kvm_lock);
> kvm_free_irq_routing(kvm);
> for (i = 0; i < KVM_NR_BUSES; i++) {
> - if (kvm->buses[i])
> - kvm_io_bus_destroy(kvm->buses[i]);
> + struct kvm_io_bus *bus;
> +
> + bus = (rcu_dereference_protected(kvm->buses[i], 1));
> + if (bus)
> + kvm_io_bus_destroy(bus);
> kvm->buses[i] = NULL;
> }
> kvm_coalesced_mmio_free(kvm);
> @@ -3572,7 +3575,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> {
> struct kvm_io_bus *new_bus, *bus;
>
> - bus = kvm->buses[bus_idx];
> + bus = rcu_dereference_protected(kvm->buses[bus_idx], 1);
> if (!bus)
> return -ENOMEM;
>
> @@ -3601,7 +3604,7 @@ void kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
> int i;
> struct kvm_io_bus *new_bus, *bus;
>
> - bus = kvm->buses[bus_idx];
> + bus = rcu_dereference_protected(kvm->buses[bus_idx], 1);
> if (!bus)
> return;
>
>
