Re: [TEST] TCP MD5 vs kmemleak
From: Catalin Marinas <catalin.marinas@arm.com>
Date: 2024-07-08 11:05:21
Also in:
rcu
On Tue, Jul 02, 2024 at 03:08:42AM +0100, Dmitry Safonov wrote:
quoted hunk ↗ jump to hunk
On Thu, 20 Jun 2024 at 18:02, Paul E. McKenney [off-list ref] wrote:quoted
So we need call_rcu() to mark memory flowing through it? If so, we need help from callers of call_rcu() in the case where more than one object is being freed.Not sure, I think one way to avoid explicitly marking pointers for call_rcu() or even avoiding the patch above would be a hackery like this:diff --git a/mm/kmemleak.c b/mm/kmemleak.c index d5b6fba44fc9..7a5eb55a155c 100644 --- a/mm/kmemleak.c +++ b/mm/kmemleak.c@@ -1587,6 +1587,7 @@ static void kmemleak_cond_resched(structkmemleak_object *object) static void kmemleak_scan(void) { struct kmemleak_object *object; + unsigned long gp_start_state; struct zone *zone; int __maybe_unused i; int new_leaks = 0;@@ -1630,6 +1631,7 @@ static void kmemleak_scan(void) kmemleak_cond_resched(object); } rcu_read_unlock(); + gp_start_state = get_state_synchronize_rcu(); #ifdef CONFIG_SMP /* per-cpu sections scanning */@@ -1690,6 +1692,13 @@ static void kmemleak_scan(void) */ scan_gray_list(); + /* + * Wait for the greylist objects potentially migrating from + * RCU callbacks or maybe getting freed. + */ + cond_synchronize_rcu(gp_start_state); + rcu_barrier(); + /* * Check for new or unreferenced objects modified since the previous * scan and color them gray until the next scan. -->8--Not quite sure if this makes sense, my first time at kmemleak code, adding Catalin. But then if I didn't mess up, it's going to work only for one RCU period, so in case some object calls rcu/kfree_rcu() from the callback, it's going to be yet a false-positive.
Yeah, I don't think this fully solves the problem.
Some other options/ideas: - more RCU-invasive option which would be adding a mapping function to segmented lists of callbacks, which would allow kmemleak to request from RCU if the pointer is yet referenced by RCU.
This looks too invasive to me plus additional scanning cost.
- the third option is for kmemleak to trust RCU and generalize the commit above, adding kmemleak_object::flags of OBJECT_RCU, which will be set by call_rcu()/kfree_rcu() and unset once the callback is invoked for RCU_DONE_TAIL.
This could work, the downside being a kmemleak object lookup every time call_rcu() is invoked. Well, we do this now for kfree_rcu() already, though we just mark the object as ignored once.
- add kmemleak_object::update_jiffies or just directly touch kmemleak_object::jiffies whenever the object has been modified (see update_checksum()), that will ignore recently changed objects. As rcu_head should be updated, that is going to automagically ignore those deferred to RCU objects.
This looks the simplest, reset the jiffies every time it detected the object being touched. However, I wonder whether we should introduce a new variable to track this. 'jiffies' currently stores the creation time (or thereabouts). We can change the 'age' reported in the sysfs as long as no user script parses that. I added the min age to kmemleak to avoid the early object being added to some lists and getting reported as false positive. It looks like we have a similar scenario when the object is getting freed: it is added to RCU lists, disappears temporarily from kmemleak's view. So it does make sense to employ similar delay as for creation. That said, for the default kmemleak operation with scanning every 10min, this wouldn't be needed. During one scan it detects the checksum changed due to the rcu_head update. 10min later it should have been freed already. -- Catalin