On Tue, Jan 10, 2023 at 06:20:33AM +0000, Tian, Kevin wrote:
> > +/**
> > + * iommufd_vfio_compat_ioas_create_id - Return the IOAS ID that vfio
>
> 'ID' is not returned in this case.
>
> and it's slightly clearer to remove the trailing '_id' in the function name.
>
> > @@ -235,6 +253,9 @@ static int iommufd_vfio_check_extension(struct
> > iommufd_ctx *ictx,
> > case VFIO_UNMAP_ALL:
> > return 1;
> >
> > + case VFIO_NOIOMMU_IOMMU:
> > + return IS_ENABLED(CONFIG_VFIO_NOIOMMU);
> > +
>
> also check vfio_noiommu?
Can't easily, that value is in another module
> Another subtle difference. The container path has below check which applies
> to noiommu:
>
> /*
> * The container is designed to be an unprivileged interface while
> * the group can be assigned to specific users. Therefore, only by
> * adding a group to a container does the user get the privilege of
> * enabling the iommu, which may allocate finite resources. There
> * is no unset_iommu, but by removing all the groups from a container,
> * the container is deprivileged and returns to an unset state.
> */
> if (list_empty(&container->group_list) || container->iommu_driver) {
> up_write(&container->group_lock);
> return -EINVAL;
> }
We don't follow that model in iommufd, once you have an iommufd you
can immediately start allocating resources, and it is not considered
"unprivileged". Instead all resource usage is constrained by the cgroup
I suppose it is something of a difference in IOMMUFD_VFIO_CONTAINER
that it behaves like this, but fixing it is not related to noiommu
> > @@ -133,12 +133,13 @@ static int vfio_group_ioctl_set_container(struct
> > vfio_group *group,
> >
> > iommufd = iommufd_ctx_from_file(f.file);
> > if (!IS_ERR(iommufd)) {
> > - u32 ioas_id;
> > -
> > - ret = iommufd_vfio_compat_ioas_id(iommufd, &ioas_id);
> > - if (ret) {
> > - iommufd_ctx_put(group->iommufd);
> > - goto out_unlock;
> > + if (!IS_ENABLED(CONFIG_VFIO_NO_IOMMU) ||
> > + group->type != VFIO_NO_IOMMU) {
> > + ret =
> > iommufd_vfio_compat_ioas_create_id(iommufd);
> > + if (ret) {
> > + iommufd_ctx_put(group->iommufd);
> > + goto out_unlock;
> > + }
>
> This doesn't prevent userspace from mixing noiommu and the check
> in vfio_iommufd_bind() is partial which only ensures no compat ioas
> when binding a noiommu device. It's still possible to bind a VFIO_IOMMU
> type device and create the compat ioas afterwards.
There are many ways to have an IOAS but also have an iommufd "bound"
to a noiommu device - userspace could just use the native iommufd
interface, or mess with the IOMMUFD_CMD_VFIO_IOAS, for instance.
I don't really want to get to the same kind of protection as vfio
typically did because it would be very invasive for little gain.
So long as a well behaved userspace does not become confused that it
thinks there is translation but none exists I think this is good
enough.
The way it is set here is if userspace somehow had a compat IOAS
assigned then things will fail, and if it attempts to do a map after
binding then it will also fail. This is enough to protect well behaved
userspace.
This will become more explicit in the cdev stuff where userspace must
explicitly specify -1 as the iommufd to run in noiommu and userspace
cannot become confused.
Jason
