On Tue, 2023-06-20 at 15:40 -0700, Paul E. McKenney wrote: > On Mon, Jun 19, 2023 at 11:13:32PM -0300, Leonardo Bras wrote: > > On Fri, 2023-06-16 at 03:39 -0300, Leonardo Bras wrote: > > > While building the CodeSamples/datastruct/Issaquah/ directory, I can > > > see > > > a couple instances of this warning: > > > > > > In function ‘free_treenode_cache’, > > > inlined from ‘tree_remove_all’ at tree.c:102:2, > > > inlined from ‘tree_free’ at tree.c:128:2: > > > tree.c:251:9: warning: ‘free’ called on pointer ‘trp’ with nonzero > > > offset 96 [-Wfree-nonheap-object] > > > 251 | free(tnp); > > > | ^~~~~~~~~ > > > > > > I took a look and tried to understand what was happening: > > > - tree_remove_all() calls free_treenode_cache() on it's input, which > > > ends > > > up free()'ing it (!BAD_MALLOC) > > > - It makes sense in most treenodes, since they are allocated with > > > alloc_treenode_cache() and the malloc() output is the same as the > > > free() > > > input. > > > - tree_free() calls tree_remove_all() on &trp->max, which ends up > > > trying > > > to free() this same address. > > > - trp is a struct treeroot, which is composed of 2 treenodes: min & > > > max > > > - The output of malloc() for trp ends up being different from the > > > address > > > used for free(), since &trp->max is used instead, and there is an > > > offset > > > since max is the second element of struct treeroot. > > > > > > To solve this while keeping the tree_remove_all() generic, move > > > struct traceroot->max to be the first element, and guarantee the > > > address > > > used for free() is the same returned by malloc(). > > > > Extra info: > > > > Bug reproduction: > > https://gitlab.com/linux-kernel/perfbook/-/jobs/4501216686#L212 > > > > With bugfix provided in this patch: > > https://gitlab.com/linux-kernel/perfbook/-/jobs/4485986705 > > Hello, Leo, and apologies for being slow. > > My feeling is that there is a deeper bug involving use of the wrong > pointer, as in freeing a pointer to a field of the enclosing structure. That's correct. I have proposed a hack, which works even though the code is still 'incorrect'. The thing is that the current code calls tree_remove_all() for &trp->max which was malloc'ed as a struct treeroot, unlike the other treenodes, and yet will try to free trp->max. > What are your thoughts on adjusting things so that the correct pointer > is freed? (And no, I have not yet looked at this closely, so there > might well be a very good reason why my suggestion is bogus. But I have > to ask!) Sure, it's possible to do that. I will create a helper _tree_remove_all() that does everything except remove the treenode, and call that on trp->max @ tree_free(), as the trp itself is freed soon after. Then I will call _tree_remove_all() + free_treenode_cache() at tree_remove_all(). This should make sure everything is right. It's a longer change, and I was unsure of how much would be ok to change in that code, but I am happy to provide v2 like this. I will send the v2 soon. Thanks, Leo > > Thanx, Paul > > > Thanks, > > Leo > > > > > > > > Signed-off-by: Leonardo Bras <leobras.c@xxxxxxxxx> > > > --- > > > CodeSamples/datastruct/Issaquah/tree.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/CodeSamples/datastruct/Issaquah/tree.h > > > b/CodeSamples/datastruct/Issaquah/tree.h > > > index f007558a..bbe5e7c1 100644 > > > --- a/CodeSamples/datastruct/Issaquah/tree.h > > > +++ b/CodeSamples/datastruct/Issaquah/tree.h > > > @@ -48,8 +48,8 @@ struct treenode { > > > * Root of a tree. > > > */ > > > struct treeroot { > > > - struct treenode min; > > > struct treenode max; > > > + struct treenode min; > > > } __attribute__((__aligned__(CACHE_LINE_SIZE))); > > > > > > void treenode_wire_call_rcu(void); > > > >
