linux-kernel - Re: [PATCH v5 0/4] New Microsemi PCI Switch Management Driver

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: <20170301235854.GF2820@obsidianresearch.com>
Date:   Wed, 1 Mar 2017 16:58:54 -0700
From:   Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:     Logan Gunthorpe <logang@...tatee.com>
Cc:     Bjorn Helgaas <helgaas@...nel.org>,
        Keith Busch <keith.busch@...el.com>,
        Myron Stowe <myron.stowe@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Jonathan Corbet <corbet@....net>,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Emil Velikov <emil.l.velikov@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Ryusuke Konishi <konishi.ryusuke@....ntt.co.jp>,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>,
        Wei Zhang <wzhang@...com>,
        Kurt Schwemmer <kurt.schwemmer@...rosemi.com>,
        Stephen Bates <stephen.bates@...rosemi.com>,
        linux-pci@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 0/4] New Microsemi PCI Switch Management Driver

On Wed, Mar 01, 2017 at 03:49:04PM -0700, Logan Gunthorpe wrote:

> Seems to me like an elegant solution would be to implement a 'cdev_kill'
> function which could kill all the processes using a cdev. Thus, during
> an unbind, a driver could call it and be sure that there are no users
> left and it can safely allow the devres unwind to continue. Then no
> difficult and racy 'alive' flags would be necessary and it would be much
> easier on drivers.

That could help, but this would mean cdev would have to insert a shim
to grab locks around the various file ops.

> > 3) A couple of drivers drivers/char/tpm doesn't seem to have any
> > protection at all and appears like they would continue to use io
> > operations even after the they may get unmapped because the char device
> > persists.

AFAIK TPM is correct and has been robustly tested now. We have a 'vtpm'
driver that agressively uses hot-unplug.

Firstly, TPM correctly tracks krefs on the 'chip', so the pointer is
always valid.

Next, anytime someone has a 'chip' pointer and wants to execute a TPM
operation they have to call tpm_try_get_ops and hold that lock for the
duration of their usage. Internally it does this:

        down_read(&chip->ops_sem);
        if (!chip->ops)
                goto out_lock; // FAILURE

In the cdev fops case this will cause the fop to return EPIPE if it
fails. cdev holds this read side lock while it is running TPM commands
inside the driver.

This meshes with this code called by tpm_chip_unregister():

        down_write(&chip->ops_sem);
        chip->ops = NULL;
        up_write(&chip->ops_sem);

tpm_chip_unregister uses this to provide a strong fence guarentee to
the driver. After tpm_chip_unregister return:
 - Nothing is running in a TPM ops callback currently
 - Nothing will ever call a TPM ops callback in future

This allows the TPM drivers to be trivial: call tpm_chip_unregister(),
kill the IRQ, and unmap the io resource, unload the module code.

TPM drivers cannot associate HW resources to the 'chip' struct device,
as it is unwound and devm triggered *during* tpm_chip_unregister and
does not enjoy the strong fence guarantee.

infiniband has a similar 'strong fence' unregister function. I think
it is best-practice for subsystem design to provide that because
drivers will never get it right on their own :(

Both infiniband and TPM have the 'detach' flavour of unplug where the
user is not forced to close their open cdevs. For simpler cases you
can just wait for all cdevs to close with a simple rwsem, much like
sysfs does already during device_del.

Jason