Re: mm: keep page cache radix tree nodes in check

[Johannes Weiner <hannes@cmpxchg.org>]

Hello Dan,

On Thu, Mar 10, 2016 at 03:59:23PM +0300, Dan Carpenter wrote:
> Hello Johannes Weiner,
> 
> The patch 449dd6984d0e: "mm: keep page cache radix tree nodes in
> check" from Apr 3, 2014, leads to the following static checker
> warning:
> 
> 	mm/filemap.c:138 page_cache_tree_delete()
> 	error: potentially using uninitialized 'node'.
> 
> mm/filemap.c
>    113  static void page_cache_tree_delete(struct address_space *mapping,
>    114                                     struct page *page, void *shadow)
>    115  {
>    116          struct radix_tree_node *node;
>                                         ^^^^
>    117          unsigned long index;
>    118          unsigned int offset;
>    119          unsigned int tag;
>    120          void **slot;
>    121  
>    122          VM_BUG_ON(!PageLocked(page));
>    123  
>    124          __radix_tree_lookup(&mapping->page_tree, page->index, &node, &slot);
>                                                                        ^^^^
>    125  
>    126          if (shadow) {
>    127                  mapping->nrexceptional++;
>    128                  /*
>    129                   * Make sure the nrexceptional update is committed before
>    130                   * the nrpages update so that final truncate racing
>    131                   * with reclaim does not see both counters 0 at the
>    132                   * same time and miss a shadow entry.
>    133                   */
>    134                  smp_wmb();
>    135          }
>    136          mapping->nrpages--;
>    137  
>    138          if (!node) {
>                      ^^^^
> 
>    139                  /* Clear direct pointer tags in root node */
>    140                  mapping->page_tree.gfp_mask &= __GFP_BITS_MASK;
>    141                  radix_tree_replace_slot(slot, shadow);
>    142                  return;
>    143          }
> 
> It's obviously simple enough for me to initialize "node" to NULL but I
> suspect there is a reason that it can't be uninitialized...  I'm trying
> to get some feedback for some new Smatch stuff I'm working on.

We know that page->tree[page->index] is present and the tree is
locked, so __radix_tree_lookup() will always return with an entry, as
well as &node and &slot set. I'm not sure how you would annotate this.

Is it also warning about slot? Or can it know that they are always set
together? Could it maybe be linked to the function's return value? I
would prefer not setting node and slot to NULL to suppress the false
positive. However, what we could do is add a BUG_ON() if the function
call returns NULL. Would that be enough of a hint to the checker that
we expect the function to be always successful and set node and slot?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>