On Fri, Jan 20, 2023 at 07:28:19PM +0000, Robin Murphy wrote: > On 2023-01-18 18:00, Jason Gunthorpe wrote: > > Change the sg_alloc_table_from_pages() allocation that was hardwired to > > GFP_KERNEL to use the gfp parameter like the other allocations in this > > function. > > > > Auditing says this is never called from an atomic context, so it is safe > > as is, but reads wrong. > > I think the point may have been that the sgtable metadata is a > logically-distinct allocation from the buffer pages themselves. Much like > the allocation of the pages array itself further down in > __iommu_dma_alloc_pages(). That makes sense, and it is a good reason to mask off the allocation policy flags from the gfp. On the other hand it also makes sense to continue to pass in things like NOWAIT|NOWARN to all the allocations. Even to the iommu driver. So I'd prefer to change this to mask and make all the following calls consistently use the input gfp > I'd say the more confusing thing about this particular context is why we're > using iommu_map_sg_atomic() further down - that seems to have been an > oversight in 781ca2de89ba, since this particular path has never supported > being called in atomic context. Huh. I had fixed that in v1, this patch was supposed to have that hunk, that was the main point of making this patch actually.. > Overall I'm starting to wonder if it might not be better to stick a "use > GFP_KERNEL_ACCOUNT if you allocate" flag in the domain for any level of the > API internals to pick up as appropriate, rather than propagate per-call gfp > flags everywhere. We might get to something like that, but it requires more parts that are not ready yet. Most likely this would take the form of some kind of 'this is an iommufd created domain' indication. This happens naturally as part of the nesting patches. Right now I want to get people to start testing with this because the charge from the IOPTEs is far and away the largest memory draw. Parts like fixing the iommu drivers to actually use gfp are necessary to make it work. If we flip the two places using KERNEL_ACCOUNT to something else later it doesn't really matter. I think the removal of the two _atomic wrappers is still appropriate stand-alone. > As it stands we're still missing potential pagetable and other > domain-related allocations by drivers in .attach_dev and even (in Yes, I plan to get to those when we add an alloc_domain_iommufd() or whatever op. The driver will know the calling context and can set the gfp flags for any allocations under alloc_domain under that time. Then we can go and figure out if there are other allocations and if all or only some drivers need a flag - eg at attach time. Though this is less worrying because you can only scale attach up to num_pasids * num open vfios. iommufd will let userspace create and populate an unlimited number of iommu_domains, so everything linked to an unattached iommu_domain should be charged. > probably-shouldn't-really-happen cases) .unmap_pages... Gah, unmap_pages isn't allow to fail. There is no way to recover from this. iommufd will spew a warn and then have a small race where userspace can UAF kernel memory. I'd call such a driver implementation broken. Why would you need to do this?? :( Thanks, Jason
