linux-kernel - Re: [PATCH 5/7] iommu/arm-smmu-v3: Allow building as a module

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]

Date:   Mon, 4 Nov 2019 20:15:24 +0100
From:   Jean-Philippe Brucker <jean-philippe@...aro.org>
To:     Will Deacon <will@...nel.org>
Cc:     Joerg Roedel <joro@...tes.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        iommu@...ts.linux-foundation.org,
        Robin Murphy <robin.murphy@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] iommu/arm-smmu-v3: Allow building as a module

On Thu, Oct 31, 2019 at 03:42:47PM +0000, Will Deacon wrote:
> > Sorry for the stupid question, but what prevents the iommu module from
> > being unloaded when there are active users? There are no symbol
> > dependencies to endpoint device drivers, because the interface is only
> > exposed through the iommu-api, right? Is some sort of manual module
> > reference counting needed?
> 
> Generally, I think unloading the IOMMU driver module while there are
> active users is a pretty bad idea, much like unbinding the driver via
> /sys in the same situation would also be fairly daft. However, I *think*
> the code in __device_release_driver() tries to deal with this by
> iterating over the active consumers and ->remove()ing them first.

> I'm without hardware access at the moment, so I haven't been able to
> test this myself. We could nobble the module_exit() hook, but there's
> still the "force unload" option depending on the .config.

Shame that we can't completely prevent module unloading, because handling
rmmod cleanly is tricky.

On module unload we also need to tidy up the bus->iommu_ops installed by
bus_set_iommu(), and remove the IOMMU groups (and probably other leaks I
missed). I have a solution for the bus->iommu_ops, which is simply adding
a bus_unset_iommu() counterpart with a refcount, but it doesn't deal with
the IOMMU groups cleanly. If there are multiple IOMMU instances managing
one bus, then we should only remove the IOMMU groups belonging to the
instance that is being removed.

I'll think about this more, but the simple solution is attached if you
want to test. It at least works with a single IOMMU now:

$ modprobe virtio-iommu
[   25.180965] virtio_iommu virtio0: input address: 64 bits
[   25.181437] virtio_iommu virtio0: page mask: 0xfffffffffffff000
[   25.214493] virtio-pci 0000:00:03.0: Adding to iommu group 0
[   25.233252] virtio-pci 0000:00:03.0: enabling device (0000 -> 0003)
[   25.334810] e1000e 0000:00:02.0: Adding to iommu group 1
[   25.348997] e1000e 0000:00:02.0: enabling device (0000 -> 0002)
... net test etc

$ rmmod virtio-iommu
[   34.084816] e1000e: eth1 NIC Link is Down
[   34.212152] pci 0000:00:02.0: Removing from iommu group 1
[   34.250558] pci 0000:00:03.0: Removing from iommu group 0
[   34.261570] virtio_iommu virtio0: device removed

$ modprobe virtio-iommu
[   34.828982] virtio_iommu virtio0: input address: 64 bits
[   34.829442] virtio_iommu virtio0: page mask: 0xfffffffffffff000
[   34.844576] virtio-pci 0000:00:03.0: Adding to iommu group 0
[   34.916449] e1000e 0000:00:02.0: Adding to iommu group 1

Thanks,
Jean

View attachment "0001-iommu-Add-bus_unset_iommu.patch" of type "text/plain" (5401 bytes)