linux-kernel - Re: [PATCH] exit: dump thread info on global init exit

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]

Message-ID: <20231110074123.GA7768@redhat.com>
Date:   Fri, 10 Nov 2023 08:41:23 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     chenqiwu <qiwuchen55@...il.com>
Cc:     axboe@...nel.dk, keescook@...omium.org, akpm@...ux-foundation.org,
        mcgrof@...nel.org, ebiederm@...ssion.com, jannh@...gle.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] exit: dump thread info on global init exit

On 11/10, chenqiwu wrote:
>
> On Thu, Nov 09, 2023 at 12:01:30PM +0100, Oleg Nesterov wrote:
> > I've just noticed we discuss this offlist. Add lkml...
> >
> > On 11/09, chenqiwu wrote:
> > >
> > > On Wed, Nov 08, 2023 at 10:57:32AM +0100, Oleg Nesterov wrote:
> > >
> > > > > +	if (mmap_read_lock_killable(mm)) {
> > > >
> > > > why do you need _killable ?
> > > >
> > > I'm not sure which type lock (killable or unkillable) should be used here
> >
> > killable should be used to allow to kill the task which waits for this lock.
> > Who can kill the global init? Yes it is possible (but very unlikely) that
> > fatal_signal_pending() is true, but I don't think this was your concern.
> >
> > > if there is a lock contention, perhaps using down_read_trylock is better.
> >
> > Perhaps. If we have another bug mmap_read_lock() can hang forever.
> >
> Yes, but we really don't want to hang here forever if we cannot get the mmap read
> lock.

Yes, this is what I tried to say.

> I think using down_read_trylock should be better, I wiil resend the patch as
> V2 cc LKML to discuss the thing.

OK.

> > > > > +static void dump_thread_info(struct task_struct *tsk)
> > > > > +{
> > > > > +	struct pt_regs *regs = task_pt_regs(tsk);
> > > > > +
> > > > > +	if (user_mode(regs))
> > > > > +		dump_thread_maps_info(tsk);
> > > > > +	show_regs(regs);
> > > >
> > > > This looks confusing to me...
> > > >
> > > > How can user_mode() return false in this case? And even if this is
> > > > possible, then show_regs() should depend on user_mode() as well?
> > > > I must have missed something.
> > > >
> > > Sure, the last global init thread cannot be exited in non-user mode.
> >
> > Forgot to mention... panic() should dump the regs, so I think show_regs()
> > is not needed?
> >
> In fact, panic don't dump the regs. For example, the current kill init panic log from Andriod system:

Hmm. OK, panic()->dump_stack() depends on CONFIG_DEBUG_BUGVERBOSE, so perhaps
Andriod runs without CONFIG_DEBUG_BUGVERBOSE... or perhaps show_stack() doesn't
do __show_regs() on arm64 ? I dunno.

OK, I see you have already sent V2.

Oleg.