On Thu, Apr 13, 2023 at 02:52:54AM +0000, Tian, Kevin wrote:
From: Jason Gunthorpe jgg@nvidia.com Sent: Wednesday, April 12, 2023 7:18 PM
On Wed, Apr 12, 2023 at 08:27:36AM +0000, Tian, Kevin wrote:
From: Jason Gunthorpe jgg@nvidia.com Sent: Tuesday, April 11, 2023 10:31 PM
On Thu, Mar 23, 2023 at 07:21:42AM +0000, Tian, Kevin wrote:
If no oversight then we can directly put the lock in iommufd_hw_pagetable_attach/detach() which can also simplify a bit
on
its callers in device.c.
So, I did this, and syzkaller explains why this can't be done:
https://lore.kernel.org/r/0000000000006e66d605f83e09bc@google.com
We can't allow the hwpt to be discovered by a parallel iommufd_hw_pagetable_attach() until it is done being setup, otherwise if we fail to set it up we can't destroy the hwpt.
if (immediate_attach) { rc = iommufd_hw_pagetable_attach(hwpt, idev); if (rc) goto out_abort; }
rc = iopt_table_add_domain(&hwpt->ioas->iopt, hwpt->domain); if (rc) goto out_detach; list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list); return hwpt;
out_detach: if (immediate_attach) iommufd_hw_pagetable_detach(idev); out_abort: iommufd_object_abort_and_destroy(ictx, &hwpt->obj);
As some other idev could be pointing at it too now.
How could this happen before this object is finalized? iirc you pointed to me this fact in previous discussion.
It only is unavailable through the xarray, but we've added it to at least one internal list on the group already, it is kind of sketchy to work like this, it should all be atomic..
which internal list? group has a list for attached devices but regarding to hwpt it's stored in a single field igroup->hwpt.
It is added to
list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
Which can be observed from
mutex_lock(&ioas->mutex); list_for_each_entry(hwpt, &ioas->hwpt_list, hwpt_item) { if (!hwpt->auto_domain) continue;
if (!iommufd_lock_obj(&hwpt->obj)) continue;
If iommufd_lock_obj() has happened then iommufd_object_abort_and_destroy() is in trouble.
Thus we need to hold the ioas->mutex right up until we know we can't call iommufd_object_abort_and_destroy(), or lift out the hwpt list_add
This could maybe also be fixed by holding the destroy_rw_sem right up until finalize. Though, I think I looked at this once and decided against it for some reason..
btw removing this lock in this file also makes it easier to support siov device which doesn't have group. We can have internal group attach and pasid attach wrappers within device.c and leave igroup->lock held in the group attach path.
Yeah, I expect this will need more work when we get to PASID support
Most likely the resolution will be something like PASID domains can't be used as PF/VF domains because they don't have the right reserved regions, so they shouldn't be in the hwpt_list at all, so we can use a more relaxed locking.
Jason