On 09/12/2018 09:43 PM, Tony Krowiak wrote:
> +/**
> + * vfio_ap_mdev_open_once
> + *
> + * @matrix_mdev: a mediated matrix device
> + *
> + * Return 0 if no other mediated matrix device has been opened for the
> + * KVM guest assigned to @matrix_mdev; otherwise, returns an error.
> + */
> +static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev,
> + struct kvm *kvm)
> +{
> + struct ap_matrix_mdev *m;
> +
> + mutex_lock(&matrix_dev->lock);
> +
> + list_for_each_entry(m, &matrix_dev->mdev_list, node) {
> + if ((m != matrix_mdev) && (m->kvm == kvm)) {
> + mutex_unlock(&matrix_dev->lock);
> + return -EPERM;
> + }
> + }
> +
> + mutex_unlock(&matrix_dev->lock);
> +
> + return 0;
> +}
> +
> +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + int ret;
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + if (action != VFIO_GROUP_NOTIFY_SET_KVM)
> + return NOTIFY_OK;
> +
> + matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
> +
> + if (!data) {
> + matrix_mdev->kvm = NULL;
> + return NOTIFY_OK;
> + }
> +
> + ret = vfio_ap_mdev_open_once(matrix_mdev, data);
This could be racy. Two threads doing vfio_ap_mdev_group_notifier()
can first get 0 here in a sense that there is no such kvm in the list,
and then both set the very same kvm three lines below. Which would
result in what we are trying to prevent.
Also vfio_ap_mdev_open_once() does not seem like an appropriate name
any more. If we were to do the matrix_mdev->kvm = kvm in there we could
call it something like vfio_ap_mdev_set_kvm().
> + if (ret)
> + return NOTIFY_DONE;
> +
> + matrix_mdev->kvm = data;
> +
> + ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm);
> + if (ret)
> + return ret;
> +
> + vfio_ap_mdev_copy_masks(matrix_mdev);
> +
> + return NOTIFY_OK;
> +}
