Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i494840e7:Fastmail
Date:   Mon, 1 May 2023 16:40:19 +0300
From:   Ido Schimmel <idosch@idosch.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Schspa Shi <schspa@gmail.com>, longman@redhat.com,
        swboyd@chromium.org, linux@roeck-us.net, wuchi.zero@gmail.com,
        linux-kernel@vger.kernel.org,
        syzbot+5093ba19745994288b53@syzkaller.appspotmail.com,
        danieller@nvidia.com, petrm@nvidia.com
Subject: Re: [PATCH] debugobject: Prevent init race with static objects
Message-ID: <ZE/BQ5yUQZUwhlTu@shredder>
References: <20230303161906.831686-1-schspa@gmail.com>
 <87bkl9jt3a.ffs@tglx>
 <m2ttycd8kx.fsf@gmail.com>
 <87sfdw8yru.ffs@tglx>
 <87pm908xvu.ffs@tglx>
 <87zg7dzgao.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87zg7dzgao.ffs@tglx>
Precedence: bulk

On Wed, Apr 12, 2023 at 09:54:39AM +0200, Thomas Gleixner wrote:
> Statically initialized objects are usually not initialized via the init()
> function of the subsystem. They are special cased and the subsystem
> provides a function to validate whether an object which is not yet tracked
> by debugobjects is statically initialized. This means the object is started
> to be tracked on first use, e.g. activation.
> 
> This works perfectly fine, unless there are two concurrent operations on
> that object. Schspa decoded the problem:

[...]

> This race exists forever, but was never observed until mod_timer() got a
> debug_object_assert_init() added which is outside of the timer base lock
> held section right at the beginning of the function to cover the lockless
> early exit points too.
> 
> Rework the code so that the lookup, the static object check and the
> tracking object association happens atomically under the hash bucket
> lock. This prevents the issue completely as all callers are serialized on
> the hash bucket lock and therefore cannot observe inconsistent state.
> 
> Fixes: 3ac7fe5a4aab ("infrastructure to debug (dynamic) objects")
> Reported-by: syzbot+5093ba19745994288b53@syzkaller.appspotmail.com
> Debugged-by: Schspa Shi <schspa@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://syzkaller.appspot.com/bug?id=22c8a5938eab640d1c6bcc0e3dc7be519d878462
> Link: https://lore.kernel.org/lkml/20230303161906.831686-1-schspa@gmail.com

Thomas,

With this patch we see the following warning in the kernel log during
boot:

"ODEBUG: Out of memory. ODEBUG disabled"

In which case, the stats are:

# cat /sys/kernel/debug/debug_objects/stats
max_chain     :24
max_checked   :37
warnings      :0
fixups        :0
pool_free     :4297
pool_pcp_free :84
pool_min_free :0
pool_used     :0
pool_max_used :6615
on_free_list  :0
objs_allocated:15616
objs_freed    :11319


If I revert this patch, the warning disappears and I see the following
stats:

# cat /sys/kernel/debug/debug_objects/stats
max_chain     :25
max_checked   :40
warnings      :0
fixups        :0
pool_free     :1219
pool_pcp_free :209
pool_min_free :289
pool_used     :1578
pool_max_used :8026
on_free_list  :0
objs_allocated:32304
objs_freed    :29507

The following diff seems to solve the problem for me:

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index b796799fadb2..af4bd66c571c 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -21,7 +21,7 @@
 #define ODEBUG_HASH_BITS       14
 #define ODEBUG_HASH_SIZE       (1 << ODEBUG_HASH_BITS)
 
-#define ODEBUG_POOL_SIZE       1024
+#define ODEBUG_POOL_SIZE       (16 * 1024)
 #define ODEBUG_POOL_MIN_LEVEL  256
 #define ODEBUG_POOL_PERCPU_SIZE        64
 #define ODEBUG_BATCH_SIZE      16

In which case, the stats are:

# cat /sys/kernel/debug/debug_objects/stats 
max_chain     :28
max_checked   :64
warnings      :0
fixups        :0
pool_free     :14789
pool_pcp_free :192
pool_min_free :8120
pool_used     :1595
pool_max_used :8264
on_free_list  :0
objs_allocated:16384
objs_freed    :0

I'm not familiar with the debugobjects code, but maybe it makes sense to
make "ODEBUG_POOL_SIZE" configurable via Kconfig in a similar fashion to
"CONFIG_DEBUG_KMEMLEAK_MEM_POOL_SIZE"?

Please let me know if more information is required or if you want me to
test a patch.

Thanks