On Thu, Nov 18, 2021 at 02:39:45AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Tuesday, November 16, 2021 9:46 PM
> >
> > On Tue, Nov 16, 2021 at 09:57:30AM +0800, Lu Baolu wrote:
> > > Hi Christoph,
> > >
> > > On 11/15/21 9:14 PM, Christoph Hellwig wrote:
> > > > On Mon, Nov 15, 2021 at 10:05:42AM +0800, Lu Baolu wrote:
> > > > > +enum iommu_dma_owner {
> > > > > + DMA_OWNER_NONE,
> > > > > + DMA_OWNER_KERNEL,
> > > > > + DMA_OWNER_USER,
> > > > > +};
> > > > > +
> > > >
> > > > > + enum iommu_dma_owner dma_owner;
> > > > > + refcount_t owner_cnt;
> > > > > + struct file *owner_user_file;
> > > >
> > > > I'd just overload the ownership into owner_user_file,
> > > >
> > > > NULL -> no owner
> > > > (struct file *)1UL) -> kernel
> > > > real pointer -> user
> > > >
> > > > Which could simplify a lot of the code dealing with the owner.
> > > >
> > >
> > > Yeah! Sounds reasonable. I will make this in the next version.
> >
> > It would be good to figure out how to make iommu_attach_device()
> > enforce no other driver binding as a kernel user without a file *, as
> > Robin pointed to, before optimizing this.
> >
> > This fixes an existing bug where iommu_attach_device() only checks the
> > group size and is vunerable to a hot plug increasing the group size
> > after it returns. That check should be replaced by this series's logic
> > instead.
> >
>
> I think this existing bug in iommu_attach_devce() is different from
> what this series is attempting to solve. To avoid breaking singleton
> group assumption there the ideal band-aid is to fail device hotplug.
> Otherwise some IOVA ranges which are supposed to go upstream
> to IOMMU may be considered as p2p and routed to the hotplugged
> device instead.
Yes, but the instability of the reserved regions during hotplug with
!ACS seems like an entirely different problem. It affects everything,
including VFIO, and multi-device groups. Certainly it is nothing to do
with this series.
> In concept a singleton group is different from a
> multi-devices group which has only one device bound to driver...
Really? Why? I don't see it that way..
A singleton group is just a multi-device group that hasn't been
hotplugged yet.
We don't seem to have the concept of a "true" singleton group which is
permanently single due to HW features.
> This series aims to avoid conflict having both user and kernel drivers
> mixed in a multi-devices group.
I see this series about bringing order to all the places that want to
use a non-default domain - in-kernel or user doesn't really matter.
ie why shouldn't iommu_attach_device() work in a group that has a PCI
bridge, just like VFIO does?
The only thing that is special about VFIO vs a kernel driver is we
want a little help to track userspace ownership and VFIO opens
userspace to do the P2P attack.
The way I see it the num device == 1 test in iommu_attach_device() is
an imperfect way of controlling driver binding, and we can do better
by using the mechanism in this series.
Jason
