[bug report] drm/i915/gvt: hold reference of VFIO group during opening of vgpu
Yan Zhao
yan.y.zhao at intel.com
Mon Aug 16 00:50:56 UTC 2021
On Fri, Aug 13, 2021 at 02:04:19PM +0300, Dan Carpenter wrote:
> Hello Yan Zhao,
>
> The patch 776d95b768e6: "drm/i915/gvt: hold reference of VFIO group
> during opening of vgpu" from Mar 12, 2020, leads to the following
> Smatch static checker warning:
>
> drivers/gpu/drm/i915/gvt/kvmgt.c:919 intel_vgpu_open_device()
> warn: 'vfio_group' is an error pointer or valid
>
> drivers/gpu/drm/i915/gvt/kvmgt.c
> 909 ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, &events,
> 910 &vdev->group_notifier);
> 911 if (ret != 0) {
> 912 gvt_vgpu_err("vfio_register_notifier for group failed: %d\n",
> 913 ret);
> 914 goto undo_iommu;
> 915 }
> 916
> 917 vfio_group = vfio_group_get_external_user_from_dev(mdev_dev(mdev));
> 918 if (IS_ERR_OR_NULL(vfio_group)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> When a function returns both NULLs and error pointers, then the NULL is
> NOT an error. This normally happens when a feature has been
> deliberately disabled:
>
> foo = get_optional_feature();
>
hi Dan,
Thanks for your mail.
The reason IS_ERR_OR_NULL was selected is that I think even the function
returns a valid NULL pointer for whatever reason, kvmgt should refuse to
handle it.
Though I agree we can turn it to IS_ERR safely for now, I'll leave it to
the maintainer to decide whether this change is deserved.
Thanks
Yan
> If the feature is disabled, then the code has to continue without
> printing an error message or crashing. If the feature has an error then
> that has to be reported to the user and we return an error code.
>
> In this case, I don't think vfio_group_get_external_user_from_dev()
> returns NULL so the fix is to just change the check to IS_ERR().
>
> --> 919 ret = !vfio_group ? -EFAULT : PTR_ERR(vfio_group);
> 920 gvt_vgpu_err("vfio_group_get_external_user_from_dev failed\n");
> 921 goto undo_register;
> 922 }
> 923 vdev->vfio_group = vfio_group;
> 924
> 925 /* Take a module reference as mdev core doesn't take
> 926 * a reference for vendor driver.
> 927 */
> 928 if (!try_module_get(THIS_MODULE)) {
> 929 ret = -ENODEV;
> 930 goto undo_group;
> 931 }
> 932
> 933 ret = kvmgt_guest_init(mdev);
> 934 if (ret)
> 935 goto undo_group;
> 936
> 937 intel_gvt_ops->vgpu_activate(vgpu);
> 938
> 939 atomic_set(&vdev->released, 0);
> 940 return ret;
> 941
> 942 undo_group:
> 943 vfio_group_put_external_user(vdev->vfio_group);
> 944 vdev->vfio_group = NULL;
> 945
> 946 undo_register:
> 947 vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> 948 &vdev->group_notifier);
> 949
> 950 undo_iommu:
> 951 vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> 952 &vdev->iommu_notifier);
> 953 out:
> 954 return ret;
> 955 }
>
> regards,
> dan carpenter
More information about the intel-gvt-dev
mailing list