On Wed, May 28, 2025 at 05:47:19PM +0530, Aneesh Kumar K.V wrote:
> +#if IS_ENABLED(CONFIG_KVM)
> +#include <linux/kvm_host.h>
> +
> +static void viommu_get_kvm_safe(struct iommufd_viommu *viommu, struct kvm *kvm)
> +{
> + void (*put_fn)(struct kvm *kvm);
> + bool (*get_fn)(struct kvm *kvm);
> + bool ret;
> +
> + if (!kvm)
> + return;
> +
> + put_fn = symbol_get(kvm_put_kvm);
> + if (WARN_ON(!put_fn))
> + return;
> +
> + get_fn = symbol_get(kvm_get_kvm_safe);
> + if (WARN_ON(!get_fn)) {
> + symbol_put(kvm_put_kvm);
> + return;
> + }
> +
> + ret = get_fn(kvm);
> + symbol_put(kvm_get_kvm_safe);
> + if (!ret) {
> + symbol_put(kvm_put_kvm);
> + return;
> + }
> +
> + viommu->put_kvm = put_fn;
> + viommu->kvm = kvm;
> +}
Shameer was working on something like this too
I would probably split just the viommu kvm stuff into one patch so you
two can share it.
> @@ -68,10 +121,32 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
> */
> viommu->iommu_dev = __iommu_get_iommu_dev(idev->dev);
>
> + /* get the kvm details if specified. */
> + if (cmd->kvm_vm_fd) {
Pedantically a 0 fd is still valid, you should add a flag to indicate
if the KVM is being supplied.
> + struct kvm *kvm;
> + struct fd f = fdget(cmd->kvm_vm_fd);
> +
> + if (!fd_file(f)) {
> + rc = -EBADF;
> + goto out_abort;
> + }
> +
> + if (!file_is_kvm(fd_file(f))) {
> + rc = -EBADF;
> + fdput(f);
> + goto out_abort;
> + }
> + kvm = fd_file(f)->private_data;
> + viommu_get_kvm_safe(viommu, kvm);
> + fdput(f);
I mentioned this to Sean a while back, but can we just use the fdget
reference here and forget about kvm_get_kvm_safe()?
> +int iommufd_vdevice_tsm_bind_ioctl(struct iommufd_ucmd *ucmd)
> +{
> + struct iommu_vdevice_id *cmd = ucmd->cmd;
> + struct iommufd_vdevice *vdev;
> + int rc = 0;
> +
> + vdev = container_of(iommufd_get_object(ucmd->ictx, cmd->vdevice_id,
> + IOMMUFD_OBJ_VDEVICE),
> + struct iommufd_vdevice, obj);
> + if (IS_ERR(vdev))
> + return PTR_ERR(vdev);
> +
> + rc = tsm_bind(vdev->dev, vdev->viommu->kvm, vdev->id);
Yeah, that makes alot of sense now, you are passing in the KVM for the
VIOMMU and both the vBDF and pBDF to the TSM layer, that should be
enough for it to figure out what to do. The only other data would be
the TSM's VIOMMU handle..
> +int iommufd_vdevice_tsm_unbind_ioctl(struct iommufd_ucmd *ucmd)
> +{
> + struct iommu_vdevice_id *cmd = ucmd->cmd;
> + struct iommufd_vdevice *vdev;
> + int rc = 0;
> +
> + vdev = container_of(iommufd_get_object(ucmd->ictx, cmd->vdevice_id,
> + IOMMUFD_OBJ_VDEVICE),
> + struct iommufd_vdevice, obj);
> + if (IS_ERR(vdev))
> + return PTR_ERR(vdev);
> +
> + rc = tsm_unbind(vdev->dev);
> + if (rc) {
> + rc = -ENODEV;
> + goto out_put_vdev;
> + }
But there is no locking here so userspace could race tsm_unbind with
tsm_bind, which doesn't sound great. Shouldn't iommufd protect against
that?
You could abuse the device_lock(vdev->dev) ?
I think we still have an existing bug where the vdevice can outlive
the idevice, but that is not your issue, just FYI
Jason
