* re: mm: keep page cache radix tree nodes in check
@ 2016-03-10 12:59 Dan Carpenter
2016-03-10 16:12 ` Johannes Weiner
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2016-03-10 12:59 UTC (permalink / raw)
To: hannes; +Cc: linux-mm
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 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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: mm: keep page cache radix tree nodes in check
2016-03-10 12:59 mm: keep page cache radix tree nodes in check Dan Carpenter
@ 2016-03-10 16:12 ` Johannes Weiner
2016-03-10 19:35 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Johannes Weiner @ 2016-03-10 16:12 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: mm: keep page cache radix tree nodes in check
2016-03-10 16:12 ` Johannes Weiner
@ 2016-03-10 19:35 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2016-03-10 19:35 UTC (permalink / raw)
To: Johannes Weiner; +Cc: linux-mm
On Thu, Mar 10, 2016 at 11:12:00AM -0500, Johannes Weiner wrote:
> 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...
> Is it also warning about slot?
It does, yes.
> Or can it know that they are always set
> together?
It knows they are set together but it warns about both.
> 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
--
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-10 19:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-10 12:59 mm: keep page cache radix tree nodes in check Dan Carpenter
2016-03-10 16:12 ` Johannes Weiner
2016-03-10 19:35 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).