netdev - Re: [TEST] TCP MD5 vs kmemleak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZovH7cWlw9SGcWoJ@arm.com>
Date: Mon, 8 Jul 2024 12:05:17 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Dmitry Safonov <0x7f454c46@...il.com>
Cc: paulmck@...nel.org, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	rcu@...r.kernel.org
Subject: Re: [TEST] TCP MD5 vs kmemleak

On Tue, Jul 02, 2024 at 03:08:42AM +0100, Dmitry Safonov wrote:
> On Thu, 20 Jun 2024 at 18:02, Paul E. McKenney <paulmck@...nel.org> wrote:
> > 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.

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