Hello Arne Jansen,
The patch 8c9c2bf7a3c4: "btrfs: fix race in reada" from Feb 25, 2012,
leads to the following warning:
fs/btrfs/reada.c:308 reada_find_zone()
warn: ''zone'' was already freed.
@@ -307,13 +302,15 @@ again:
ret = radix_tree_insert(&dev->reada_zones,
(unsigned long)(zone->end >>
PAGE_CACHE_SHIFT),
zone);
- spin_unlock(&fs_info->reada_lock);
- if (ret) {
+ if (ret == -EEXIST) {
kfree(zone);
^^^^^^^^^^^
Freed here.
- looped = 1;
- goto again;
+ ret = radix_tree_gang_lookup(&dev->reada_zones, (void
**)&zone,
^^^^
Use after free inside radix_tree_gang_lookup() function.
+ logical >> PAGE_CACHE_SHIFT,
1);
+ if (ret == 1)
+ kref_get(&zone->refcnt);
}
+ spin_unlock(&fs_info->reada_lock);
return zone;
}
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30.04.2012 13:11, Dan Carpenter wrote:> Hello Arne Jansen, > > The patch 8c9c2bf7a3c4: "btrfs: fix race in reada" from Feb 25, 2012, > leads to the following warning: > fs/btrfs/reada.c:308 reada_find_zone() > warn: ''zone'' was already freed.Who emits this warning? It''s bogus.> > @@ -307,13 +302,15 @@ again: > ret = radix_tree_insert(&dev->reada_zones, > (unsigned long)(zone->end >> PAGE_CACHE_SHIFT), > zone); > - spin_unlock(&fs_info->reada_lock); > > - if (ret) { > + if (ret == -EEXIST) { > kfree(zone); > ^^^^^^^^^^^ > Freed here. > > - looped = 1; > - goto again; > + ret = radix_tree_gang_lookup(&dev->reada_zones, (void **)&zone, > ^^^^ > Use after free inside radix_tree_gang_lookup() function.It''s not used by radix_tree_gang_lookup, the second parameter is a pointer to the return value. Thanks, Arne> > + logical >> PAGE_CACHE_SHIFT, 1); > + if (ret == 1) > + kref_get(&zone->refcnt); > } > + spin_unlock(&fs_info->reada_lock); > > return zone; > } > > regards, > dan carpenter >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 30, 2012 at 01:23:29PM +0200, Arne Jansen wrote:> On 30.04.2012 13:11, Dan Carpenter wrote: > > Hello Arne Jansen, > > > > The patch 8c9c2bf7a3c4: "btrfs: fix race in reada" from Feb 25, 2012, > > leads to the following warning: > > fs/btrfs/reada.c:308 reada_find_zone() > > warn: ''zone'' was already freed. > > Who emits this warning? It''s bogus. >This is a Smatch warning, but it''s not turned on by default because a lot of place do this: kfree(p); printk(KERN_DEBUG "pointer was %p", p);> > > > @@ -307,13 +302,15 @@ again: > > ret = radix_tree_insert(&dev->reada_zones, > > (unsigned long)(zone->end >> PAGE_CACHE_SHIFT), > > zone); > > - spin_unlock(&fs_info->reada_lock); > > > > - if (ret) { > > + if (ret == -EEXIST) { > > kfree(zone); > > ^^^^^^^^^^^ > > Freed here. > > > > - looped = 1; > > - goto again; > > + ret = radix_tree_gang_lookup(&dev->reada_zones, (void **)&zone, > > ^^^^ > > Use after free inside radix_tree_gang_lookup() function. > > It''s not used by radix_tree_gang_lookup, the second parameter is > a pointer to the return value. >Uh... I just glanced at it the first time through, but looking but look carefully, we''re not using freed memory, we''re scribbling over stack memory. Eep! 990 unsigned int 991 radix_tree_gang_lookup(struct radix_tree_root *root, void **results, 992 unsigned long first_index, unsigned int max_items) 993 { 994 struct radix_tree_iter iter; 995 void **slot; 996 unsigned int ret = 0; 997 998 if (unlikely(!max_items)) 999 return 0; 1000 1001 radix_tree_for_each_slot(slot, root, &iter, first_index) { 1002 results[ret] = indirect_to_ptr(rcu_dereference_raw(*slot)); ^^^^^^^^^^^^ The bug is right here. The first time through we write to *(&zone + 0) which is just "zone = indirect_to_ptr();" and that''s fine. But the second time we write to "*(&zone + 1)" which corrupts the cache pointer from the reada_find_zone() function. My guess is that the normal use case is to only loop through here one time or we would have caught this in testing. All the uses in fs/btrfs/reada.c have same memory corruption issue. 1003 if (!results[ret]) 1004 continue; 1005 if (++ret == max_items) 1006 break; 1007 } 1008 1009 return ret; 1010 } regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 30, 2012 at 01:23:29PM +0200, Arne Jansen wrote:> On 30.04.2012 13:11, Dan Carpenter wrote: > > Hello Arne Jansen, > > > > The patch 8c9c2bf7a3c4: "btrfs: fix race in reada" from Feb 25, 2012, > > leads to the following warning: > > fs/btrfs/reada.c:308 reada_find_zone() > > warn: ''zone'' was already freed. > > Who emits this warning? It''s bogus. > > > > > @@ -307,13 +302,15 @@ again: > > ret = radix_tree_insert(&dev->reada_zones, > > (unsigned long)(zone->end >> PAGE_CACHE_SHIFT), > > zone); > > - spin_unlock(&fs_info->reada_lock); > > > > - if (ret) { > > + if (ret == -EEXIST) { > > kfree(zone); > > ^^^^^^^^^^^ > > Freed here. > > > > - looped = 1; > > - goto again; > > + ret = radix_tree_gang_lookup(&dev->reada_zones, (void **)&zone, > > ^^^^ > > Use after free inside radix_tree_gang_lookup() function. > > It''s not used by radix_tree_gang_lookup, the second parameter is > a pointer to the return value.Ah. We can only write one item, because we pass max_items = 1. Nevermind. The code in reada.c is fine. Sorry for the noise. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html