linux-kernel - Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm

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, 19 Oct 2017 17:26:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Michal Suchánek <msuchanek@...e.de>
Cc:     SF Markus Elfring <elfring@...rs.sourceforge.net>,
        linux-integrity@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        kernel-janitors@...r.kernel.org,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>,
        Nayna Jain <nayna@...ux.vnet.ibm.com>,
        Jerry Snitselaar <jsnitsel@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
        Corentin Labbe <clabbe.montjoie@...il.com>,
        Kenneth Goldman <kgold@...ux.vnet.ibm.com>,
        Paul Mackerras <paulus@...ba.org>,
        Peter Hüwe <PeterHuewe@....de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after
 error detection

tpm_ibmvtpm_probe() is an example of poor names.  It has the generic
ones like "cleanup" which don't say *what* is cleaned and the come-from
ones like "init_irq_cleanup" which don't say anything useful at all:

   647          rc = request_irq(vio_dev->irq, ibmvtpm_interrupt, 0,
   648                           tpm_ibmvtpm_driver_name, ibmvtpm);
   649          if (rc) {
   650                  dev_err(dev, "Error %d register irq 0x%x\n", rc, vio_dev->irq);
   651                  goto init_irq_cleanup;
   652          }
   653  
   654          rc = vio_enable_interrupts(vio_dev);
   655          if (rc) {
   656                  dev_err(dev, "Error %d enabling interrupts\n", rc);
   657                  goto init_irq_cleanup;
   658          }

Sadly, we never do call free_irq().

> It can do it automatically, in most cases better, and automatically
> adapt it to code changes.

I've heard this before that if you only have one label that does
everything then it's "automatic" and "future proof".  It's not true.
The best error handling is methodical error handling:

1) In the reverse order from how things were allocated
2) That mirrors the allocations exactly
3) That frees one thing at a time
4) With a proper, useful, readable label name which says what the goto
   does
5) That doesn't free anything which hasn't been allocated

regards,
dan carpenter