Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Jun 2023 15:13:24 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Dmitry Antipov <dmantipov@yandex.ru>
Cc:     Anna Schumaker <anna@kernel.org>,
        Chuck Lever <chuck.lever@oracle.com>,
        Jeff Layton <jlayton@kernel.org>, Neil Brown <neilb@suse.de>,
        Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, linux-nfs@vger.kernel.org,
        llvm@lists.linux.dev, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] SUNRPC: clean up integer overflow check
Message-ID: <ac54a100-9e7f-47bf-a415-ca3784c08b1e@kadam.mountain>
References: <2390fdc8-13fa-4456-ab67-44f0744db412@moroto.mountain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2390fdc8-13fa-4456-ab67-44f0744db412@moroto.mountain>
Precedence: bulk

We were kind of having a discussion about static checkers earlier...

The point of static checkers is to find code that doesn't behave as the
authors intended.  It can be tricky to determine what is intentional or
not.

If you have a condition which can never be true then that's fairly
suspicious.  Why would we write a condition like that?  But you have to
look at the context.

	if (!p) {

Maybe that's part of a macro and even though "p" can't be NULL here, it
can be NULL in other places.  So we can't warning about bogus NULL
checks if they are inside a macro.  There is a valid reason to check.

With NULL checks, people add a lot of unnecessary NULL checks just out
of caution.  So even though the NULL checks are unnecessary, they aren't
harmful.  You only want to warn about them if there is something else to
make them suspicious.  For example, the if the pointer is an error
pointer but you're checking for NULL then probably that's not
intentional.  Or people think that list_for_each() loops exit with the
iterator set to NULL but that's not how it works so those checks should
trigger a warning.

Maybe we have a condition "if (unsigned_val < 0", and that's very
suspicious, but maybe the context is:

	if (unsigned_val < 0 || unsigned_val > 10)

In that context we can see what the intention was, and that condition
will clamp the value regardless of the type of unsigned_val so it works
as intended.  It's not a bug.  The static checker should not warn about
that.

Another impossible condition could look like:

	if (x & SOMETHING_ZERO) {

Quite often the SOMETHING_ZERO is because it depends on the .config.  So
in Smatch what I do is I make a list of macros which can be zero or not
depending on the .config.  In a lot of cases, what was intended was:

	if (x & (1 << SOMETHING_ZERO)) {

That's a useful warning, but you have to create that list of allowed
defines otherwise it generates too many false positives.

So here we have:

	if (u32_val > SOMETHING) {

The condition is impossible when the code is compiled on a 64-bit
system, but a human reading the code can see that it is an integer
overflow check and Smatch can see that too.  I haven't looked at Clang
or GCC internals to see how hard it would be to fix them.  Maybe it's
really hard to fix?

With static checkers we have to accept the checker is going to get it
wrong some times.  The checker is going to warn about code that works
as intended and it's going to miss over 97% of bugs.  But hopefully
it generates enough warnings to be worth the time it takes and the
number of false positives is small enough so you can keep up.

regards,
dan carpenter