linux-kernel - Re: [PATCH v2 1/3] iommufd: Add devices_users to track the hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y9rW01eROepZuMt3@Asurada-Nvidia>
Date:   Wed, 1 Feb 2023 13:18:03 -0800
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     <kevin.tian@...el.com>, <yi.l.liu@...el.com>,
        <iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] iommufd: Add devices_users to track the
 hw_pagetable usage by device

On Wed, Feb 01, 2023 at 04:00:40PM -0400, Jason Gunthorpe wrote:
> On Wed, Feb 01, 2023 at 11:25:10AM -0800, Nicolin Chen wrote:
> 
> > The "finalise" is one of the very first lines of the attach_dev()
> > callback function in SMMU driver, though it might still undesirably
> > fail the replace().
> 
> It won't get that far.
> 
> Remember how this all works - only autodomains have the special path
> that allocates a domain, attaches the empty domain, and then populates
> it with the ioas. We made this special path specifically to accomodate
> the current ARM drivers, otherwise they wouldn't work at all.

Yes.

> replace can't do this - replace must always start out with a
> pre-existing hwpt that was allocated with a dedicated hwpt allocation
> ioctl.
> 
> Wwhen the hwpt was allocated it must be linked to the IOAS at that
> time, because we definately don't do defered IOAS linkage.
>
> So on ARM when you create an unfinalizes iommu_domain it cannot be
> added to the IOAS (because it has an empty aperture) and creation will
> fail, or worse, it will get added to an empty IOAS and make the IOAS
> permanently unusable.

IIUIC, user space might add some IOVA mappings to the hwpt/iopt,
before doing a replace(). If we do a deferred IOAS linkage to
this hwpt/iopt, it will cause a problem because we are adding
the reserved region for the MSI window later than IOMMU_IOAS_MAP
calls. Thus, "we definately don't do defered IOAS linkage".

With this justification, I think I should include my patch of
moving iopt_table_add/remove_domain(), in the replace series.

> > Hmm...in that case, we should hold two ioas->mutex locks in
> > addition to two device locks?
> 
> No, the device lock is the thing that protects the data you are
> touching no reason to make it any different.

Thinking it deeper: we don't really touch the old_hwpt->ioas or
its iopt, but only the new_hwpt->ioas/iopt to reserve the MSI
window, so there isn't an actual need to hold old_hwpt->ioas's
mutex.

I will prepare the replace series and send it by the end of the
day, upon certain level of sanity/verification.

Thanks!
Nic