Received-SPF: pass (google.com: domain of linux-kernel+bounces-170352-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 6 May 2024 21:42:33 +0200 (CEST)
From: Julia Lawall <julia.lawall@inria.fr>
To: Lukas Wunner <lukas@wunner.de>
cc: Nicolas Palix <nicolas.palix@imag.fr>, cocci@inria.fr, 
    linux-kernel@vger.kernel.org
Subject: Re: coccinelle matching of identifiers
In-Reply-To: <ZjkZBvuXlZAodPAU@wunner.de>
Message-ID: <alpine.DEB.2.22.394.2405062136410.3284@hadrien>
References: <ZjkZBvuXlZAodPAU@wunner.de>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII


On Mon, 6 May 2024, Lukas Wunner wrote:

> Dear coccinelle maintainers,
>
> Linux kernel commit 5c6ca9d93665 ("X.509: Introduce scope-based
> x509_certificate allocation"), which is queued for v6.10 in this repo ...
>
>     https://git.kernel.org/herbert/cryptodev-2.6/c/5c6ca9d93665
>
> ... triggers scripts/coccinelle/null/eno.cocci:
>
>     ./crypto/asymmetric_keys/x509_cert_parser.c:69:9-15: ERROR: allocation function on line 68 returns NULL not ERR_PTR on failure
>     ./fs/gfs2/inode.c:1850:6-12: ERROR: allocation function on line 1842 returns NULL not ERR_PTR on failure
>     ./fs/smb/client/cifsfs.c:1186:6-12: ERROR: allocation function on line 1173 returns NULL not ERR_PTR on failure
>
> The first of these three errors is newly introduced by the above-linked
> commit, the other two already existed before.  All are false-positives.
>
> I would like to silence the newly-introduced false-positive and have
> attempted to do so using the small patch below.
>
> However the result is that *only* the newly-introduced false-positive is
> found and the other two are no longer found.  So the other way round than
> what I'm aiming for.
>
> I find this bewildering.  What am I doing wrong?
>
> FWIW, coccinelle version is 1.1.1.
>
> The newly introduced false-positive is triggered by the statement:
>
>     assume(!IS_ERR(cert));
>
> Which is a macro that expands to:
>
>     do { if (!!IS_ERR(cert)) __builtin_unreachable(); } while(0);
>
> The macro gives the compiler a hint that variable "cert" is not an error
> pointer, which prevents the compiler from adding an unnecessary check
> for that condition.
>
> Thanks!
>
> Lukas
>
> -- >8 --
>
> diff --git a/scripts/coccinelle/null/eno.cocci b/scripts/coccinelle/null/eno.cocci
> index 7107d6c8db9e..79112d51bee8 100644
> --- a/scripts/coccinelle/null/eno.cocci
> +++ b/scripts/coccinelle/null/eno.cocci
> @@ -26,10 +26,12 @@ x = \(kmalloc\|kzalloc\|kcalloc\|kmem_cache_alloc\|kmem_cache_zalloc\|kmem_cache
>  @r depends on !patch exists@
>  expression x,E;
>  position p1,p2;
> +identifier assume;
>  @@
>
>  *x = \(kmalloc@p1\|kzalloc@p1\|kcalloc@p1\|kmem_cache_alloc@p1\|kmem_cache_zalloc@p1\|kmem_cache_alloc_node@p1\|kmalloc_node@p1\|kzalloc_node@p1\)(...)
>  ... when != x = E
> +    when != assume
>  * IS_ERR@p2(x)

The problem is that ... is searching along control-flow paths, and
Coccinelle only considers control-flow at the statement level, not within
expressions.

Maybe a reasonable solution would be just to consider that we really just
want to protect against if tests?  So

*x = \(kmalloc@p1\|kzalloc@p1\|kcalloc@p1\|kmem_cache_alloc@p1\|kmem_cache_zalloc@p1\|kmem_cache_alloc_node@p1\|kmalloc_node@p1\|kzalloc_node@p1\)(...)
.. when != x = E
* if ( <+... IS_ERR@p2(x) ... +> )
  S1 else S2

where S1 and S2 are declared as statement metavariables.

Another option is:

x = \(kmalloc@p1\|kzalloc@p1\|kcalloc@p1\|kmem_cache_alloc@p1\|kmem_cache_zalloc@p1\|kmem_cache_alloc_node@p1\|kmalloc_node@p1\|kzalloc_node@p1\)(...)
.. when != x = E
(
assume(!IS_ERR(x));
|
* IS_ERR(x)
)

In this case, only the IS_ERR is marked with a *, because if we also
marked the initial assignment, that would get highlighted regardless of
which branch of the disjunction was matched.

julia