linux-kernel - Re: [PATCH] debugobjects: Move printk out of db lock critical sections

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] [day] [month] [year] [list]

Date:   Fri, 14 Dec 2018 13:21:13 -0500
From:   Waiman Long <longman@...hat.com>
To:     Dmitry Safonov <dima@...sta.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Yang Shi <yang.shi@...ux.alibaba.com>,
        Arnd Bergmann <arnd@...db.de>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH] debugobjects: Move printk out of db lock critical
 sections

On 12/14/2018 01:06 PM, Dmitry Safonov wrote:
> On 12/13/18 10:10 PM, Waiman Long wrote:
>> The bucket lock is for protecting the insertion and deletion of
>> debug_obj to/from the bucket list as well as searching within the bucket
>> list. It has nothing to do with the life time of the debug_obj itself.
> The bucket lock also protects lookups on a bucket.

Yes, that is what I meant by "searching".

>
> 1. Imagine, you have object in ODEBUG_STATE_DESTROYED.
> If you will try debug_object_activate() with this patch, it will debug
> print object outside of bucket lock, which means that
> debug_check_no_obj_freed() may concurrently fixup/free object and
> meta-data on another CPU.
>
> I don't see this state is being used in many places around the kernel,
> except selftest and i915 driver.
>
> 2. If you try to deactivate already non-active object - you will have
> debug print outside of bucket lock. The other CPU can fixup/free page
> with this object concurrently.
>
> (before your patch the lookup would fail and no printing)
>
> So, I might be mistaken, I'm mostly worried about dereferencing the
> descriptor inside of the object during printing.
> And in my opinion, i.e., there was a reason to save `descr` before
> releasing the bucket lock here:
> https://elixir.bootlin.com/linux/latest/source/lib/debugobjects.c#L789
>
> Anyway, I see that your patch is already in -mm tree and Thomas seems to
> be fine with the change so whatever %)

I also thought about whether we should save descr before unlock. Anyway,
we can always send a follow-up patch to make further change if it is the
right thing to do.

The debugobjects code is enabled in a debug kernel and never in a kernel
for production use. The printk should only be triggered if there is a
bug in the code which the debug object is tracking.

Thanks,
Longman