On Wed, 2023-06-21 at 01:26 -0300, Leonardo Brás wrote: > 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. > Or even simpler, add a new bool parameter: free node. On tree root, just pass a false so it is not freed. This is much simpler, and even adding a new parameter, should not be much of an overhead, since this is not a hot-path. Thanks! Leo > 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); > > > > > > > >
