linux-kernel - Re: [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read)

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: <ZFIe7dLEncWN5YaQ@codewreck.org>
Date:   Wed, 3 May 2023 17:44:29 +0900
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Christian Schoenebeck <linux_oss@...debyte.com>
Cc:     Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Simon Horman <simon.horman@...igine.com>, v9fs@...ts.linux.dev,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 5/5] 9p: remove dead stores (variable set again
 without being read)

Christian Schoenebeck wrote on Wed, May 03, 2023 at 10:22:46AM +0200:
> On Wednesday, May 3, 2023 9:49:29 AM CEST Dominique Martinet wrote:
> > The 9p code for some reason used to initialize variables outside of the
> > declaration, e.g. instead of just initializing the variable like this:
> > 
> > int retval = 0
> > 
> > We would be doing this:
> > 
> > int retval;
> > retval = 0;
> 
> OK, but AFAICS this patch would simply remove all initializations. I would
> expect at least a default initialization at variable declaration instead.

Yes, clang doesn't seem to complain about 'int reval = 0' so the patch
can just be updated to do that instead; I just removed them because the
sheer number made it faster to do that.
Happy to drop this last patch for now and rework it when time permits.

> > This is perfectly fine and the compiler will just optimize dead stores
> > anyway, but scan-build seems to think this is a problem and there are
> > many of these warnings making the output of scan-build full of such
> > warnings:
> > fs/9p/vfs_inode.c:916:2: warning: Value stored to 'retval' is never read [deadcode.DeadStores]
> >         retval = 0;
> >         ^        ~
> 
> Honestly I don't see much value in this warning. Can't we just disable this
> warning for 9p code or is this just controllable for the entire project?

Dead stores in itself is a useful warning, it's what found the real bug
where return value was lost in patch 1 of this series, I don't think we
should just disable the warning.

-- 
Dominique Martinet | Asmadeus