Re: [PATCH 3/3] mm/page_owner: track page free call chain

[Sergey Senozhatsky]

On (07/04/16 16:29), Joonsoo Kim wrote:
[..]
> > well, yes. current hits bad_page(), page_owner helps to find out who
> > stole and spoiled it from under current.
> > 
> > CPU a							CPU b
> > 
> > 	alloc_page()
> > 	put_page() << legitimate
> > 							alloc_page()
> > err:
> > 	put_page() << legitimate, again.
> > 	           << but is actually buggy.
> > 
> > 							put_page() << double free. but we need
> > 								   << to report put_page() from
> > 								   << CPU a.
> 
> Okay. I think that this patch make finding offending user easier
> but it looks like it is a partial solution to detect double-free.
> See following example.
> 
> CPU a							CPU b
> 
> 	alloc_page()
> 	put_page() << legitimate
>  							alloc_page()
> err:
> 	put_page() << legitimate, again.
> 	           << but is actually buggy.
> 
> 	alloc_page()
> 
> 							put_page() <<
> 							legitimate,
> 							again.
> 	put_page() << Will report the bug and
> 	        page_owner have legitimate call stack.

good case. I think it will report "put_page()" from CPU b (the path that
actually dropped page refcount to zero and freed it), and alloc_page()
from CPU a. _might_ sound like a clue.

I agree, there are cases when this approach will not work out perfectly.
tracing refcount modification is probably the only reliable solution,
but given that sometimes it's unclear how to reproduce the bug, one can
end up looking at tons of traces.

> In kasan, quarantine is used to provide some delay for real free and
> it makes use-after-free detection more robust. Double-free also can be
> benefit from it. Anyway, I will not object more since it looks
> the simplest way to improve doublue-free detection for the page
> at least for now.

thanks!

there are things in the patch (it's an RFC after all) that I don't like.
in particular, I cut the corner in __dump_page_owner(). it now shows the
same text for both _ALLOC and _FREE handlers. I didn't want to add
additional ->order to page_ext. I can update the text to, e.g.
		page allocated via order ...	page_ext->order
and
		page freed, WAS allocated via order ...   page_ext->order

or extend page_ext and keep alloc and free ->order separately.
do you have any preferences here?

	-ss