netdev - Re: [patch] net: moxa: fix an error code

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: <4115115.4JnjLJnnpJ@wuerfel>
Date:	Wed, 02 Mar 2016 13:42:41 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Jonas Jensen <jonas.jensen@...il.com>,
	Luis de Bethencourt <luis@...ethencourt.com>,
	françois romieu <romieu@...zoreil.com>,
	netdev@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [patch] net: moxa: fix an error code

On Wednesday 02 March 2016 15:15:26 Dan Carpenter wrote:
> On Wed, Mar 02, 2016 at 12:36:05PM +0100, Arnd Bergmann wrote:
> > The uninitialized warning here is about a type mismatch preventing
> > gcc from noticing that two conditions are the same, I'm not sure
> > if this is a bug in gcc, or required by the C standard.
> 
> I wouldn't call it a bug, because everyone has to make trade offs
> between how fast the program runs and how accurate it is.  And trade
> offs between how ambitious your warnings are vs how many false positives
> you can tolerate.
> 
> Anyway, I feel like we should just work around GCC on a case by case
> basis instead of always using PTR_ERR_OR_ZERO().  The next version of
> GCC will fix some false positives and introduce new ones...  Next time
> using PTR_ERR_OR_ZERO() could cause warnings instead of fixing them.

It depends on whether we think the PTR_ERR_OR_ZERO() actually makes
the code more readable too. I've actually come to like it now,
the main downside being that it looks a lot like IS_ERR_OR_NULL()
which is very bad style and should be avoided at all cost. ;-)

I can also see how PTR_ERR_OR_ZERO() is easier for the compiler
to understand than IS_ERR()+PTR_ERR() and can't think of a case
where it would result in worse object code or extra (false
positive) warnings.

> Smatch works in a different way and it parse the code correctly.  But
> Smatch is slow and sometimes runs out of memory and gives up trying to
> parse large functions.  Smatch sees the two returns and tries to figure
> out the implications of each (uninitialized vs initialized).  If you
> change the code to:
> 
>         error = PTR_ERR_OR_ZERO(hash);
> 
>         if (!error)
>                 *leaf_out = be64_to_cpu(*(hash + index));
> 
>         return error;
> 
> then Smatch still breaks that up into two separate returns which imply
> initialized vs uninitialized.

Right, so it does the right thing, and it presumably understands
that 'if (error)' is the same as 'if (error < 0)' and
'if (IS_ERR_VALUE(error)', right? I think that is where gcc
gets it wrong.

	Arnd