linux-kernel - Re: [PATCH] kthread: kthread_should

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: <20231128174012.bfno4f3matn3d7cw@moria.home.lan>
Date:   Tue, 28 Nov 2023 12:40:12 -0500
From:   Kent Overstreet <kent.overstreet@...ux.dev>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org, Nicholas Piggin <npiggin@...il.com>,
        Mike Christie <michael.christie@...cle.com>,
        Zqiang <qiang1.zhang@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] kthread: kthread_should_stop() checks if we're a kthread

On Tue, Nov 28, 2023 at 10:30:49AM +0100, Petr Mladek wrote:
> Adding Andrew into Cc. He usually takes changes in kernel/kthread.c.
> 
> On Mon 2023-11-20 17:15:03, Kent Overstreet wrote:
> > bcachefs has a fair amount of code that may or may not be running from a
> > kthread (it may instead be called by a userspace ioctl); having
> > kthread_should_stop() check if we're a kthread enables a fair bit of
> > cleanup and makes it safer to use.
> > 
> > Signed-off-by: Kent Overstreet <kent.overstreet@...ux.dev>
> > ---
> >  kernel/kthread.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 1eea53050bab..fe6090ddf414 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -155,7 +155,8 @@ void free_kthread_struct(struct task_struct *k)
> >   */
> >  bool kthread_should_stop(void)
> >  {
> > -	return test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags);
> > +	return (current->flags & PF_KTHREAD) &&
> > +		test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags);
> >  }
> >  EXPORT_SYMBOL(kthread_should_stop);
> 
> I agree that it makes the API more safe because &to_kthread(current)
> is NULL when the process is not a kthread.
> 
> Well, I do not like the idea of quietly ignoring a misuse of
> the kthread_*() API. I would personally prefer to do:

It's only a misuse in the most pedantic sense, IMO.

Is it ever possible that with this change calling kthread_should_stop()
from a non-kthread could cause a problem?

> 
> // define this in include/linux/kthread.h
> static inline bool in_kthread(void)
> {
> 	return current->flags & PF_KTHREAD
> }
> 
> // add WARN() into kthread_should_stop()
> bool kthread_should_stop(void)
> {
> 	if (WARN_ON_ONCE(!in_kthread))
> 		return false;
> 
> 	return test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags);
> }
> EXPORT_SYMBOL(kthread_should_stop);
> 
> 
> And use the following in bcachefs() code:
> 
> 	if (in_kthread() && kthread_should_stop())
> 		goto exit;

That's what bcachefs code does today, and forgetting to check
in_kthread() is a real footgun - particularly for other people starting
to work on the code.

So I could do a bch2_in_kthread() instead that does the in_kthread(
check, but then I have to make sure that people know to use
bch2_in_kthread() instead of in_kthread(). Not ideal.