Re: [TEST] TCP MD5 vs kmemleak

Hi Paul,

Sorry for the delayed answer, I got a respiratory infection, so was in
bed last week,

On Thu, 20 Jun 2024 at 18:02, Paul E. McKenney [off-list ref] wrote:

On Wed, Jun 19, 2024 at 01:33:36AM +0100, Dmitry Safonov wrote:

[..]

quoted

I'm sorry guys, that was me being inadequate.

I know that feeling!

Thanks :)

[adding/quoting back the context of the thread that I cut previously]

On Tue, Jun 18, 2024 at 10:02:10AM -0700, Jakub Kicinski wrote:

quoted

On Tue, 18 Jun 2024 09:42:35 -0700 Paul E. McKenney wrote:

quoted

quoted

FTR, with mptcp self-tests we hit a few kmemleak false positive on RCU
freed pointers, that where addressed by to this patch:

commit 5f98fd034ca6fd1ab8c91a3488968a0e9caaabf6
Author: Catalin Marinas [off-list ref]
Date:   Sat Sep 30 17:46:56 2023 +0000

    rcu: kmemleak: Ignore kmemleak false positives when RCU-freeing objects

I'm wondering if this is hitting something similar? Possibly due to
lazy RCU callbacks invoked after MSECS_MIN_AGE???

Dmitry mentioned this commit, too, but we use the same config for MPTCP
tests, and while we repro TCP AO failures quite frequently, mptcp
doesn't seem to have failed once.

quoted

Fun!  ;-)

This commit handles memory passed to kfree_rcu() and friends, but
not memory passed to call_rcu() and friends.  Of course, call_rcu()
does not necessarily know the full extent of the memory passed to it,
for example, if passed a linked list, call_rcu() will know only about
the head of that list.

There are similar challenges with synchronize_rcu() and friends.

To be clear I think Dmitry was suspecting kfree_rcu(), he mentioned
call_rcu() as something he was expecting to have a similar issue but
it in fact appeared immune.

On Thu, 20 Jun 2024 at 18:02, Paul E. McKenney [off-list ref] wrote:

quoted

That's a real issue, rather than a false-positive:
https://lore.kernel.org/netdev/20240619-tcp-ao-required-leak-v1-1-6408f3c94247@gmail.com/ (local)

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(struct

kmemleak_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.

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.
- 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.
- 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.

Not sure if I mis-looked anything and it seems there is no hurry in
fixing anything yet, as this is a theoretical issue at this moment.

Hopefully, these ideas don't look like a nonsense,
             Dmitry