TCP MD5 vs kmemleak

Hi Dmitry!

We added kmemleak checks to the selftest runners, TCP AO/MD5 tests seem to trip it:

https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/643761/4-unsigned-md5-ipv6/stdout

Could you take a look? kmemleak is not infallible, it could be a false positive.

Hi Jakub,

thanks for pinging,

Quoted 10 lines · Jakub Kicinski

Hi Dmitry!

We added kmemleak checks to the selftest runners, TCP AO/MD5 tests seem to trip it:

https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/643761/4-unsigned-md5-ipv6/stdout

Could you take a look? kmemleak is not infallible, it could be a false positive.

Sure, that seems somewhat interesting, albeit at this moment not from the TCP side :D

There is some pre-history to the related issue here:
https://lore.kernel.org/lkml/[email protected]/

Which I was quite sure being addressed with what now is
https://git.kernel.org/linus/5f98fd034ca6

But now that I look at that commit, I see that kvfree_call_rcu() is defined to __kvfree_call_rcu() under CONFIG_KASAN_GENERIC=n. And I don't see the same kmemleak_ignore() on that path.

To double-check, you don't have kasan enabled on netdev runners, right?

And then straight away to another thought: the leak-report that you get currently is ao_info, which should not happen if kfree_rcu() is properly fixed.
But I'd expect kmemleak to be unhappy with ao keys freeing as well:
they are currently released with call_rcu(&key->rcu, tcp_ao_key_free_rcu), which doesn't have a hint for kmemleak, too.

I'm going to take a look at it this week. Just to let you know, I'm also looking at fixing those somewhat rare flakes on tcp-ao counters checks (but that may be net-next material together with tracepoint selftests).

Quoted 22 lines · On Tue, 18 Jun 2024 04:24:08 +0100 Dmitry Safonov wrote:

Hi Jakub,

thanks for pinging,

Quoted 10 lines · Jakub Kicinski

Hi Dmitry!

We added kmemleak checks to the selftest runners, TCP AO/MD5 tests seem to trip it:

https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/643761/4-unsigned-md5-ipv6/stdout

Could you take a look? kmemleak is not infallible, it could be a false positive.

Sure, that seems somewhat interesting, albeit at this moment not from the TCP side :D

There is some pre-history to the related issue here:
https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Which I was quite sure being addressed with what now is
https://git.kernel.org/linus/5f98fd034ca6

But now that I look at that commit, I see that kvfree_call_rcu() is defined to __kvfree_call_rcu() under CONFIG_KASAN_GENERIC=n. And I don't see the same kmemleak_ignore() on that path.

To double-check, you don't have kasan enabled on netdev runners, right?

We do:

CONFIG_KASAN=y
CONFIG_KASAN_GENERIC=y

here's the full config:
https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/645202/config

Quoted 7 lines

And then straight away to another thought: the leak-report that you get currently is ao_info, which should not happen if kfree_rcu() is properly fixed.
But I'd expect kmemleak to be unhappy with ao keys freeing as well:
they are currently released with call_rcu(&key->rcu, tcp_ao_key_free_rcu), which doesn't have a hint for kmemleak, too.

Quoted 4 lines

I'm going to take a look at it this week. Just to let you know, I'm also looking at fixing those somewhat rare flakes on tcp-ao counters checks (but that may be net-next material together with tracepoint selftests).

Let me add rcu@ to CC, perhaps folks there can guide us on known false positives with KASAN + kmemleak?

Quoted 22 lines · On Tue, 18 Jun 2024 04:24:08 +0100 Dmitry Safonov wrote:

Hi Jakub,

thanks for pinging,

Quoted 10 lines · Jakub Kicinski

Hi Dmitry!

We added kmemleak checks to the selftest runners, TCP AO/MD5 tests seem to trip it:

https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/643761/4-unsigned-md5-ipv6/stdout

Could you take a look? kmemleak is not infallible, it could be a false positive.

Sure, that seems somewhat interesting, albeit at this moment not from the TCP side :D

There is some pre-history to the related issue here:
https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Which I was quite sure being addressed with what now is
https://git.kernel.org/linus/5f98fd034ca6

But now that I look at that commit, I see that kvfree_call_rcu() is defined to __kvfree_call_rcu() under CONFIG_KASAN_GENERIC=n. And I don't see the same kmemleak_ignore() on that path.

To double-check, you don't have kasan enabled on netdev runners, right?

We do:

CONFIG_KASAN=y
CONFIG_KASAN_GENERIC=y

here's the full config:
https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/645202/config

Quoted 7 lines

And then straight away to another thought: the leak-report that you get currently is ao_info, which should not happen if kfree_rcu() is properly fixed.
But I'd expect kmemleak to be unhappy with ao keys freeing as well:
they are currently released with call_rcu(&key->rcu, tcp_ao_key_free_rcu), which doesn't have a hint for kmemleak, too.

Quoted 4 lines

I'm going to take a look at it this week. Just to let you know, I'm also looking at fixing those somewhat rare flakes on tcp-ao counters checks (but that may be net-next material together with tracepoint selftests).

Let me add rcu@ to CC, perhaps folks there can guide us on known false positives with KASAN + kmemleak?

Quoted 47 lines · Jakub Kicinski

Quoted 22 lines · On Tue, 18 Jun 2024 04:24:08 +0100 Dmitry Safonov wrote:

Hi Jakub,

thanks for pinging,

Quoted 10 lines · Jakub Kicinski

Hi Dmitry!

We added kmemleak checks to the selftest runners, TCP AO/MD5 tests seem to trip it:

https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/643761/4-unsigned-md5-ipv6/stdout

Could you take a look? kmemleak is not infallible, it could be a false positive.

Sure, that seems somewhat interesting, albeit at this moment not from the TCP side :D

There is some pre-history to the related issue here:
https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Which I was quite sure being addressed with what now is
https://git.kernel.org/linus/5f98fd034ca6

But now that I look at that commit, I see that kvfree_call_rcu() is defined to __kvfree_call_rcu() under CONFIG_KASAN_GENERIC=n. And I don't see the same kmemleak_ignore() on that path.

To double-check, you don't have kasan enabled on netdev runners, right?

We do:

CONFIG_KASAN=y
CONFIG_KASAN_GENERIC=y

here's the full config:
https://netdev-3.bots.linux.dev/vmksft-tcp-ao-dbg/results/645202/config

Quoted 7 lines

And then straight away to another thought: the leak-report that you get currently is ao_info, which should not happen if kfree_rcu() is properly fixed.
But I'd expect kmemleak to be unhappy with ao keys freeing as well:
they are currently released with call_rcu(&key->rcu, tcp_ao_key_free_rcu), which doesn't have a hint for kmemleak, too.

Quoted 7 lines

Quoted 4 lines

I'm going to take a look at it this week. Just to let you know, I'm also looking at fixing those somewhat rare flakes on tcp-ao counters checks (but that may be net-next material together with tracepoint selftests).

Let me add rcu@ to CC, perhaps folks there can guide us on known false positives with KASAN + kmemleak?

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 <[email protected]>
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???

Quoted 11 lines

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 <[email protected]>
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???

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.

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

Quoted 11 lines

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 <[email protected]>
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 8 lines

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.

Quoted 1 line

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.

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

Quoted 11 lines

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 <[email protected]>
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 8 lines

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.

Quoted 1 line

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.

Quoted 1 line · Jakub Kicinski

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

Quoted 4 lines

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 4 lines

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.

Whew!!! ;-)

Quoted 3 lines · Paul E. McKenney

Quoted 1 line · Jakub Kicinski

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

[..]

Quoted 4 lines

Quoted 4 lines

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 1 line

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

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.

Whew!!! ;-)

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

That's a real issue, rather than a false-positive:
https://lore.kernel.org/netdev/[email protected]/

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

Quoted 1 line

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

I know that feeling!

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

Quoted 11 lines

Quoted 11 lines

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 <[email protected]>
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 9 lines

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.

Quoted 2 lines

That's a real issue, rather than a false-positive:
https://lore.kernel.org/netdev/[email protected]/

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.

Quoted 3 lines · Paul E. McKenney

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:

mm/kmemleak.c +1 -0

index d5b6fba44fc9..7a5eb55a155c 100644
@@ -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;

mm/kmemleak.c +8 -1

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

Hi Paul,

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

Quoted 2 lines · Paul E. McKenney

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

[..]

Quoted 3 lines

Quoted 1 line

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]

Quoted 31 lines · Jakub Kicinski

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

Quoted 11 lines

Quoted 11 lines

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 <[email protected]>
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 9 lines

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.

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines

That's a real issue, rather than a false-positive:
https://lore.kernel.org/netdev/[email protected]/

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:

mm/kmemleak.c +1 -0

index d5b6fba44fc9..7a5eb55a155c 100644
@@ -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;

mm/kmemleak.c +1 -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 */

mm/kmemleak.c +0 -1

        /*
         * 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