re: fib_trie: Add tnode struct as a container for fields not needed in key_vector

re: fib_trie: Add tnode struct as a container for fields not needed in key_vector

[Dan Carpenter]

Hello Alexander Duyck,

The patch dc35dbeda3e0: "fib_trie: Add tnode struct as a container
for fields not needed in key_vector" from Mar 6, 2015, leads to the
following static checker warnings:

net/ipv4/fib_trie.c:330 leaf_new() warn: variable dereferenced before check 'kv' (see line 328)
net/ipv4/fib_trie.c:358 tnode_new() warn: variable dereferenced before check 'tnode' (see line 350)
net/ipv4/fib_trie.c:853 resize() error: we previously assumed 'tp' could be null (see line 835)
net/ipv4/fib_trie.c:1891 fib_trie_get_first() warn: variable dereferenced before check 't' (see line 1889)

net/ipv4/fib_trie.c
   325  static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
   326  {
   327          struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
   328          struct key_vector *l = kv->kv;
                                       ^^^^^^
Deref.

   329  
   330          if (!kv)
                    ^^^
Check.

   331                  return NULL;
   332  
   333          /* initialize key vector */
   334          l->key = key;
   335          l->pos = 0;
   336          l->bits = 0;
   337          l->slen = fa->fa_slen;
   338  
   339          /* link leaf to fib alias */
   340          INIT_HLIST_HEAD(&l->leaf);
   341          hlist_add_head(&fa->fa_list, &l->leaf);
   342  
   343          return l;
   344  }

[ snip ]

   833          while (should_inflate(tp, tn) && max_work) {
   834                  tp = inflate(t, tn);
   835                  if (!tp) {
                            ^^^
Is NULL here.

   836  #ifdef CONFIG_IP_FIB_TRIE_STATS
   837                          this_cpu_inc(stats->resize_node_skipped);
   838  #endif
   839                          break;
   840                  }
   841  
   842                  max_work--;
   843                  tn = get_child(tp, cindex);
   844          }
   845  
   846          /* Return if at least one inflate is run */
   847          if (max_work != MAX_WORK)
   848                  return node_parent(tn);
   849  
   850          /* Halve as long as the number of empty children in this
   851           * node is above threshold.
   852           */
   853          while (should_halve(tp, tn) && max_work) {
                        ^^^^^^^^^^^
should_halve() used to check for NULL but now it dereferences
unconditionally.

   854                  tp = halve(t, tn);
   855                  if (!tp) {
   856  #ifdef CONFIG_IP_FIB_TRIE_STATS
   857                          this_cpu_inc(stats->resize_node_skipped);
   858  #endif
   859                          break;
   860                  }
   861  
   862                  max_work--;
   863                  tn = get_child(tp, cindex);
   864          }

regards,
dan carpenters 

Re: fib_trie: Add tnode struct as a container for fields not needed in key_vector

[Alexander Duyck]

On 03/10/2015 02:04 PM, Dan Carpenter wrote:
> Hello Alexander Duyck,
>
> The patch dc35dbeda3e0: "fib_trie: Add tnode struct as a container
> for fields not needed in key_vector" from Mar 6, 2015, leads to the
> following static checker warnings:
>
> net/ipv4/fib_trie.c:330 leaf_new() warn: variable dereferenced before check 'kv' (see line 328)
> net/ipv4/fib_trie.c:358 tnode_new() warn: variable dereferenced before check 'tnode' (see line 350)

These two I am aware of, they are both false errors.  I am not actually 
dereferencing so much as getting a pointer to an offset inside the 
structure.  Think of it as an inverse offsetof().

> net/ipv4/fib_trie.c:853 resize() error: we previously assumed 'tp' could be null (see line 835)
This one is a valid bug, I'll address it shortly.

> net/ipv4/fib_trie.c:1891 fib_trie_get_first() warn: variable dereferenced before check 't' (see line 1889)

This is another complaint about me dereferencing, likely due to the fact 
that it missed the fact that I am just getting the pointer to the array 
of size 1.  I don't actually perform the dereferencing until later when 
I use the "->" operator on the pointer.

- Alex

Re: fib_trie: Add tnode struct as a container for fields not needed in key_vector

[Dan Carpenter]

On Tue, Mar 10, 2015 at 02:19:48PM -0700, Alexander Duyck wrote:
> 
> On 03/10/2015 02:04 PM, Dan Carpenter wrote:
> >Hello Alexander Duyck,
> >
> >The patch dc35dbeda3e0: "fib_trie: Add tnode struct as a container
> >for fields not needed in key_vector" from Mar 6, 2015, leads to the
> >following static checker warnings:
> >
> >net/ipv4/fib_trie.c:330 leaf_new() warn: variable dereferenced before check 'kv' (see line 328)
> >net/ipv4/fib_trie.c:358 tnode_new() warn: variable dereferenced before check 'tnode' (see line 350)
> 
> These two I am aware of, they are both false errors.  I am not
> actually dereferencing so much as getting a pointer to an offset
> inside the structure.  Think of it as an inverse offsetof().

Oh, right.  I always forget about this possibility.  I'll see if I can
fix the checker to not warn about these.

regards,
dan carpenter

Re: fib_trie: Add tnode struct as a container for fields not needed in key_vector

[Alexander Duyck]

On 03/10/2015 02:24 PM, Dan Carpenter wrote:
> On Tue, Mar 10, 2015 at 02:19:48PM -0700, Alexander Duyck wrote:
>> On 03/10/2015 02:04 PM, Dan Carpenter wrote:
>>> Hello Alexander Duyck,
>>>
>>> The patch dc35dbeda3e0: "fib_trie: Add tnode struct as a container
>>> for fields not needed in key_vector" from Mar 6, 2015, leads to the
>>> following static checker warnings:
>>>
>>> net/ipv4/fib_trie.c:330 leaf_new() warn: variable dereferenced before check 'kv' (see line 328)
>>> net/ipv4/fib_trie.c:358 tnode_new() warn: variable dereferenced before check 'tnode' (see line 350)
>> These two I am aware of, they are both false errors.  I am not
>> actually dereferencing so much as getting a pointer to an offset
>> inside the structure.  Think of it as an inverse offsetof().
> Oh, right.  I always forget about this possibility.  I'll see if I can
> fix the checker to not warn about these.
>
> regards,
> dan carpenter

If not you can can probably squelch it by changing them to &kv->kv[0] 
which might make it obvious to the checker.  If you can't fix the 
checker feel free to submit a patch to just massage the code to make it 
happy since the addition of a few additional operators in this case 
should make no functional difference.

- Alex