mm: keep page cache radix tree nodes in check

DISCUSSION Dan Carpenter 2 participants 3 messages Mar 10, 2016 Memory management

Share & tools

Dan Carpenter

author

2016-03-10 12:59 UTC #0 lore raw

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.

regards,
dan carpenter

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

Johannes Weiner

2016-03-10 16:12 UTC #1 lore raw

Hello Dan,

Quoted 46 lines · Dan Carpenter

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          }

Quoted 3 lines

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?

Dan Carpenter

author

2016-03-10 19:35 UTC #2 lore raw

Quoted 4 lines · Johannes Weiner

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.

That's tricky...

Quoted 1 line

Is it also warning about slot?

It does, yes.

Quoted 2 lines

Or can it know that they are always set
together?

It knows they are set together but it warns about both.

Quoted 5 lines

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?

I'm sort of just trying to get a feel for what the issues are. Calling BUG_ON() would silence the warning, yes.

regards,
dan carpenter