linux-kernel - Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after

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: <20170307190731.GD21358@bhelgaas-glaptop.roam.corp.google.com>
Date:   Tue, 7 Mar 2017 13:07:31 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Jess Frazelle <me@...sfraz.com>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Keith Busch <keith.busch@...el.com>,
        "open list:Hyper-V CORE AND DRIVERS" <devel@...uxdriverproject.org>,
        "open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        kernel-hardening@...ts.openwall.com,
        Kees Cook <keescook@...omium.org>,
        Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init

On Thu, Feb 16, 2017 at 03:38:05PM +0100, Thomas Gleixner wrote:
> On Thu, 16 Feb 2017, Bjorn Helgaas wrote:
> > On Wed, Feb 15, 2017 at 10:16:32PM +0100, Thomas Gleixner wrote:
> > 
> > > I think I suggested to Jiang to do that 'update with default functions' to
> > > 
> > > - avoid exporting the world and some more
> > > 
> > > - have the flexibility to add new functions to the ops w/o updating a
> > >   gazillion of existing usage sites, which has saved us lots of chaising in
> > >   the last years
> > > 
> > > - avoid the if (ops->ptr) ops->ptr(); else default_fn(); constructs all
> > >   over the place.
> > > 
> > > I admit I did not think about the fact that this makes the structs non
> > > const.
> > > 
> > > Mopping that up by exporting the default functions and setting all the
> > > function pointers is tedious and requires a full tree sweep when we add new
> > > stuff. There's also code shared between PCI/platform/DT based stuff, so
> > > that becomes interesting.
> > 
> > It's legal to initialize a field multiple times, and the last one
> > takes precedence, so doing this might at least avoid the full tree
> > sweeps:
> > 
> >   static struct msi_domain_ops vmd_msi_domain_ops = {
> >     MSI_DOMAIN_DEFAULT_OPS,
> >     .get_hwirq = vmd_get_hwirq,
> >   };
> > 
> > The functions referenced by MSI_DOMAIN_DEFAULT_OPS would still have to
> > be exported, though.
> 
> Hmm, that'd work. Though it will fall apart for those pieces where we share
> code across backends. But I did not yet go through all the places and check
> them.
> 
> > > Doing the if (ops->ptr) ops->ptr() else default_fn(); dance should be
> > > simpler to pull off. There are not that many sites to look at, but then we
> > > have some of the GICv3 code using the domain ops out of core.
> > > 
> > > For now doing the __ro_after_init is definitely the simplest and fastest
> > > solution to tighten these statically allocated structures.
> > 
> > I'm OK with __ro_after_init, at least as an interim solution.
> > 
> > I do think it would be good to audit all the uses of
> > MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS, since they
> > seem to be the primary indicator of when the struct might be modified.
> > I suspect we could add __ro_after_init to more than just pci-hyperv.c,
> > vmd.c, and msi.c
> 
> Agreed. I have it on my radar.

This seems like a worthwhile change, so I don't want to just drop this
patch.  But if we're going to do something, I'd like to do it
everywhere that it makes sense, all at the same time.

It looks like the v2 series was split up by subsystem, which is fine
with me.  I'll happily apply the PCI parts (or ack them, since it
might make sense to apply all of them via the same non-PCI tree).

But I *would* like to include the following users of
MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS at the same
time (or explain why __ro_after_init won't work for them):

  pci-xgene-msi.c
  pcie-altera-msi.c
  pcie-iproc-msi.c
  pcie-xilinx-nwl.c

Bjorn