linux-kernel - Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc

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: <20171019125558.itidc4ejr6goadci@mwanda>
Date:   Thu, 19 Oct 2017 15:55:59 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Michal Suchánek <msuchanek@...e.de>
Cc:     SF Markus Elfring <elfring@...rs.sourceforge.net>,
        linuxppc-dev@...ts.ozlabs.org,
        Alex Williamson <alex.williamson@...hat.com>,
        Alexey Kardashevskiy <aik@...abs.ru>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Bart Van Assche <bart.vanassche@...disk.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        David Gibson <david@...son.dropbear.id.au>,
        Doug Ledford <dledford@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Johan Hovold <johan@...nel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nathan Fontenot <nfont@...ux.vnet.ibm.com>,
        Paul Mackerras <paulus@...ba.org>,
        Rob Herring <robh@...nel.org>,
        Sahil Mehta <sahilmehta17@...il.com>,
        Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>,
        kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable
 initialisation in iommu_pseries_alloc_group()

On Thu, Oct 19, 2017 at 01:37:18PM +0200, Michal Suchánek wrote:
> Hello,
> 
> On Wed, 18 Oct 2017 21:24:25 +0200
> SF Markus Elfring <elfring@...rs.sourceforge.net> wrote:
> 
> > From: Markus Elfring <elfring@...rs.sourceforge.net>
> > Date: Wed, 18 Oct 2017 19:14:39 +0200
> > 
> > The variable "table_group" will be set to an appropriate pointer.
> > Thus omit the explicit initialisation at the beginning.
> > 
> > Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> > ---
> >  arch/powerpc/platforms/pseries/iommu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > b/arch/powerpc/platforms/pseries/iommu.c index
> > b37d4fb20d1c..b6c12b8e3ace 100644 ---
> > a/arch/powerpc/platforms/pseries/iommu.c +++
> > b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
> >  
> >  static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> >  {
> > -	struct iommu_table_group *table_group = NULL;
> > +	struct iommu_table_group *table_group;
> >  	struct iommu_table *tbl = NULL;
> >  	struct iommu_table_group_link *tgl = NULL;
> >  
> 
> I think initializing pointers to NULL is generally a good idea.
> 
> If there is no use of the variable before it is reinitialized by
> allocation gcc is free to optimize out the variable and its initial
> value.
> 
> On the other hand, if the code is changed later and use of the variable
> becomes possible you may crash (and get a gcc warning, too).

No, it's the opposite. GCC doesn't warn about potential NULL
dereferences, it warns about uninitialized variables.  By initializing
it to a bogus value, you're deliberately disabling static analysis.
We do see bugs where, if only people didn't initialize stuff to bogus
values, then the bug would have been caught before it was merged.

You might imagine that static analysis tools would catch NULL
dereferences but it's actually really really hard.  We used to have
an __uninitialized_var() macro which was used to silence GCC false
positives, but now we initialize the pointers to NULL instead.  So
most of the code that you're dealing with is stuff that was marked as
too hard for GCC to understand.  It's tricky.

regards,
dan carpenter