linux-kernel - Re: [PATCH 2/2] thunderbolt: Initialize after IOMMUs

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:   Thu, 6 Sep 2018 10:13:37 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <YehezkelShB@...il.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>
Subject: Re: [PATCH 2/2] thunderbolt: Initialize after IOMMUs

On Wed, Sep 05, 2018 at 12:46:02PM +0300, Mika Westerberg wrote:
> On Wed, Sep 05, 2018 at 10:47:46AM +0200, Lukas Wunner wrote:
> > On Mon, Sep 03, 2018 at 04:20:12PM +0300, Mika Westerberg wrote:
> > > If IOMMU is enabled and Thunderbolt driver is built into the kernel
> > > image, it will be probed before IOMMUs are attached to the PCI bus.
> > > Because of this DMA mappings the driver does will not go through IOMMU
> > > and start failing right after IOMMUs are enabled.
> > > 
> > > For this reason move the Thunderbolt driver initialization happen at
> > > rootfs level.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > > ---
> > >  drivers/thunderbolt/nhi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > > index 88cff05a1808..5cd6bdfa068f 100644
> > > --- a/drivers/thunderbolt/nhi.c
> > > +++ b/drivers/thunderbolt/nhi.c
> > > @@ -1191,5 +1191,5 @@ static void __exit nhi_unload(void)
> > >  	tb_domain_exit();
> > >  }
> > >  
> > > -fs_initcall(nhi_init);
> > > +rootfs_initcall(nhi_init);
> > >  module_exit(nhi_unload);
> > 
> > I think the dependency on the IOMMU should be open coded by returning
> > -EPROBE_DEFER from the ->probe hook if it's not yet attached.
> > Shuffling around initcall order is just applying duct tape.
> 
> It is not a dependency. The same thing can happen with any other driver
> if they happen to initialize any DMA with the device before IOMMUs are
> initialized.
> 
> > Commit acb40d841257 already changed module_init() to fs_initcall()
> > and now it has to be changed again.  Shows how fragile this is.
> 
> It is a bit fragile but I don't see any other way to handle this than
> trusting on the link ordering. Both -EPROBE_DEFER and device_links are
> out of the question AFAICT.

So with this patch, you rely on the linker ordering nhi_init() after
ir_dev_scope_init(), however to the best of my knowledge the link
order is not guaranteed.

In that sense, commit acb40d841257 was already flawed because it
executed nhi_init() at "fs" initcall level, the *same* level used by
map_properties() in drivers/firmware/efi/apple-properties.c, which
retrieves the DROM device property needed by tb_drom_copy_efi().

That was arguably a regression which the above patch cures because
"rootfs" is guaranteed to run after "fs".  Still, the fragility
remains that ir_dev_scope_init() isn't guaranteed to run before
nhi_init().

Looking at commit acb40d841257, which started this, I'm wondering
why you did not simply export tbnet_init() and call it from the
thunderbolt driver after the property stuff has been fully set up?
After all, thunderbolt-net is useless without thunderbolt or am I
missing something?  Then you could revert back to module_init().

Thanks,

Lukas