bpf, btf: Make if test explicit to fix Coccinelle error

Commit Message

Thorsten Blum June 24, 2024, 7:54 p.m. UTC

Explicitly test the iterator variable i > 0 to fix the following
Coccinelle/coccicheck error reported by itnull.cocci:

	ERROR: iterator variable bound on line 4688 cannot be NULL

Compile-tested only.

Signed-off-by: Thorsten Blum <[e-mail]>
---
 kernel/bpf/btf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexei Starovoitov June 24, 2024, 8:15 p.m. UTC | #1

On Mon, Jun 24, 2024 at 12:56 PM Thorsten Blum <[e-mail]> wrote:
>
> Explicitly test the iterator variable i > 0 to fix the following
> Coccinelle/coccicheck error reported by itnull.cocci:
>
>         ERROR: iterator variable bound on line 4688 cannot be NULL
>
> Compile-tested only.
>
> Signed-off-by: Thorsten Blum <[e-mail]>
> ---
>  kernel/bpf/btf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 821063660d9f..7720f8967814 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4687,7 +4687,7 @@ static void btf_datasec_show(const struct btf *btf,
>                             __btf_name_by_offset(btf, t->name_off));
>         for_each_vsi(i, t, vsi) {
>                 var = btf_type_by_id(btf, vsi->type);
> -               if (i)
> +               if (i > 0)
>                         btf_show(show, ",");

Sorry, I don't think this is a sustainable approach.
We cannot fix such things all over the kernel.
Pls make cocci smarter instead.

pw-bot: cr

Eduard Zingerman June 24, 2024, 8:16 p.m. UTC | #2

On Mon, 2024-06-24 at 21:54 +0200, Thorsten Blum wrote:
> Explicitly test the iterator variable i > 0 to fix the following
> Coccinelle/coccicheck error reported by itnull.cocci:
> 
> 	ERROR: iterator variable bound on line 4688 cannot be NULL
> 
> Compile-tested only.
> 
> Signed-off-by: Thorsten Blum <[e-mail]>
> ---
>  kernel/bpf/btf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 821063660d9f..7720f8967814 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4687,7 +4687,7 @@ static void btf_datasec_show(const struct btf *btf,
>  			    __btf_name_by_offset(btf, t->name_off));
>  	for_each_vsi(i, t, vsi) {
>  		var = btf_type_by_id(btf, vsi->type);
> -		if (i)
> +		if (i > 0)
>  			btf_show(show, ",");
>  		btf_type_ops(var)->show(btf, var, vsi->type,
>  					data + vsi->offset, bits_offset, show);

Could you please elaborate a bit?
Here is for_each_vsi is defined:

#define for_each_vsi(i, datasec_type, member)			\
	for (i = 0, member = btf_type_var_secinfo(datasec_type);	\
	     i < btf_type_vlen(datasec_type);			\
	     i++, member++)

Here it sets 'i' to zero for the first iteration.
Why would the tool report that 'i' can't be zero?

Thorsten Blum June 24, 2024, 11:08 p.m. UTC | #3

On 24. Jun 2024, at 13:16, Eduard Zingerman <[e-mail]> wrote:
> On Mon, 2024-06-24 at 21:54 +0200, Thorsten Blum wrote:
>> Explicitly test the iterator variable i > 0 to fix the following
>> Coccinelle/coccicheck error reported by itnull.cocci:
>> 
>> ERROR: iterator variable bound on line 4688 cannot be NULL
>> 
>> Compile-tested only.
>> 
>> Signed-off-by: Thorsten Blum <[e-mail]>
>> ---
>> kernel/bpf/btf.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 821063660d9f..7720f8967814 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -4687,7 +4687,7 @@ static void btf_datasec_show(const struct btf *btf,
>>    __btf_name_by_offset(btf, t->name_off));
>> for_each_vsi(i, t, vsi) {
>> var = btf_type_by_id(btf, vsi->type);
>> - if (i)
>> + if (i > 0)
>> btf_show(show, ",");
>> btf_type_ops(var)->show(btf, var, vsi->type,
>> data + vsi->offset, bits_offset, show);
> 
> Could you please elaborate a bit?
> Here is for_each_vsi is defined:
> 
> #define for_each_vsi(i, datasec_type, member) \
> for (i = 0, member = btf_type_var_secinfo(datasec_type); \
>     i < btf_type_vlen(datasec_type); \
>     i++, member++)
> 
> Here it sets 'i' to zero for the first iteration.
> Why would the tool report that 'i' can't be zero?

Coccinelle thinks i can't be a NULL pointer (not the number zero). It's
essentially a false-positive warning, but since there are only 4 such
warnings under kernel/, I thought it would be worthwhile to remove some
of them by making the tests explicit.

Eduard Zingerman June 24, 2024, 11:27 p.m. UTC | #4

On Mon, 2024-06-24 at 16:08 -0700, Thorsten Blum wrote:
> On 24. Jun 2024, at 13:16, Eduard Zingerman <[e-mail]> wrote:
> > On Mon, 2024-06-24 at 21:54 +0200, Thorsten Blum wrote:
> > > Explicitly test the iterator variable i > 0 to fix the following
> > > Coccinelle/coccicheck error reported by itnull.cocci:
> > > 
> > > ERROR: iterator variable bound on line 4688 cannot be NULL

[...]

> > #define for_each_vsi(i, datasec_type, member) \
> > for (i = 0, member = btf_type_var_secinfo(datasec_type); \
> >     i < btf_type_vlen(datasec_type); \
> >     i++, member++)
> > 
> > Here it sets 'i' to zero for the first iteration.
> > Why would the tool report that 'i' can't be zero?
> 
> Coccinelle thinks i can't be a NULL pointer (not the number zero). It's
> essentially a false-positive warning, but since there are only 4 such
> warnings under kernel/, I thought it would be worthwhile to remove some
> of them by making the tests explicit.

Sorry, not really familiar with the tool, but it seems like the
following part of the itnull.cocci fires the warning:

  @r depends on !patch exists@
  iterator I;
  expression x,E;
  position p1,p2;
  @@
  
  *I@p1(x,...)
  { ... when != x = E
  (
  *  x@p2 == NULL
  |
  *  x@p2 != NULL
  )
    ... when any
  }
  
  @script:python depends on org@
  p1 << r.p1;
  p2 << r.p2;
  @@
  
  cocci.print_main("iterator-bound variable",p1)
  cocci.print_secs("useless NULL test",p2)
  
  @script:python depends on report@
  p1 << r.p1;
  p2 << r.p2;
  @@
  
  msg = "ERROR: iterator variable bound on line %s cannot be NULL" % (p1[0].line)
  coccilib.report.print_report(p2[0], msg)

Is there a way to add a constraint here, requiring 'x' to have a pointer type?
(So that the rule does not match, as it clearly shouldn't).

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 821063660d9f..7720f8967814 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4687,7 +4687,7 @@  static void btf_datasec_show(const struct btf *btf,
 			    __btf_name_by_offset(btf, t->name_off));
 	for_each_vsi(i, t, vsi) {
 		var = btf_type_by_id(btf, vsi->type);
-		if (i)
+		if (i > 0)
 			btf_show(show, ",");
 		btf_type_ops(var)->show(btf, var, vsi->type,
 					data + vsi->offset, bits_offset, show);