On 09/18/2018 01:00 PM, Halil Pasic wrote:
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().
I'm moving the matrix-mdev->kvm = kvm inside the mutex lock in vfio_ap_mdev_open_once() ... also renaming it to 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; +}
