linux-kernel - Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

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:   Sun, 17 Oct 2021 21:12:17 +0100
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     Martin Kaiser <lists@...ser.cx>
Cc:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Larry Finger <Larry.Finger@...inger.net>,
        Michael Straube <straube.linux@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread

On Sun, Oct 17, 2021 at 08:02:37PM +0200, Martin Kaiser wrote:
> Hi Fabio and all,
> 
> Thus wrote Fabio M. De Francesco (fmdefrancesco@...il.com):
> 
> > On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
> 
> > > So I myself am a little confused on this one :-)
> 
> > > Based on my understanding, so correct me if I'm wrong, a process
> > > (kthread or otherwise) can still be killed if marked TASK_KILLABLE,
> > > even if ignoring SIGTERM. Indeed, from a userspace perspective,
> > > SIGKILL is unblockable anyway - although of course kernel code can
> > > choose how to respond to it.
> 
> > Correct.
> 
> And it seems that by default, a kthread can't be killed with SIGKILL.
> 

Ah, makes sense. This was a misconception on my part, apologies :-)

> > > So in other words, the kthread could still be killed while waiting
> > > in the wait_for_completion_killable() call, even if we are ignoring
> > > SIGTERM. From that perspective I guess, it is therefore not 'incorrect' as
> > > such - if indeed we wanted that behaviour.
> 
> > No. This misunderstandings is my fault. :(
> 
> > In Martin's patch I read "SIGTERM" but for some reason I thought he was 
> > talking of "SIGKILL".
> 
> > At the moment, without Martin's patch, the kthread can be terminated by the 
> > command "kill -TERM <PID>". If we try "kill -KILL <PID>", nothing happens.
> > This is because only "allow_signal(SIGTERM);" is present in the code.
> 
> Exactly. And this is probably not by intention. It would be consistent
> to either allow both or none - the latter makes more sense, and it's
> what most other drivers do.
> 
> > I think that kthreads must also allow  SIGKILL with "allow_signal(SIGKILL);" 
> > for allowing root to make them terminate.
> 
> Probably. nfsd seems to do this.
> 
> > For what relates to my patch, it doesn't matter if I either leave 
> > wait_for_completion_killable() as-is or change it to wait_for_completion().
> > This is because at the moment SIGKILL cannot kill rtw_cmd_thread(), while 
> > SIGTERM can.
> 
> > However, for consistency, I should better change it to the uninterruptible 
> > version.
> 
> That makes sense to me.
> 
> Let's see what Greg and others say...

I've found this discussion to be most enlightening :-) I certainly agree
that consistency in approach is a good thing, so perhaps
wait_for_completion() is a better choice therefore, despite it having
the same semantics in this instance due to us not allowing SIGKILL by
default anyway. Others will know far more than me in this regard though.

Regards,
Phil