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

From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Phillip Potter <phil@philpotter.co.uk>, Martin Kaiser <martin@kaiser.cx>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Larry Finger <Larry.Finger@lwfinger.net>,
	Michael Straube <straube.linux@gmail.com>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread
Date: Sun, 17 Oct 2021 15:19:28 +0200	[thread overview]
Message-ID: <2705995.aOCT9ph2Oq@localhost.localdomain> (raw)
In-Reply-To: <YWv67ozbJGxMa69t@equinox>

On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
> On Sat, Oct 16, 2021 at 08:53:15PM +0200, Fabio M. De Francesco wrote:
> > On Saturday, October 16, 2021 8:13:43 PM CEST Martin Kaiser wrote:
> > > At the moment, our command thread can be killed by user space.
> > > 
> > > [root@host ]# kill `pidof RTW_CMD_THREAD`
> > > 
> > > The driver will then stop working until the module is unloaded
> > > and reloaded.
> > > 
> > > Don't process SIGTERM in the command thread. Other drivers that have a
> > > command thread don't process SIGTERM either.
> > 
> > Hi Martin,
> > 
> > This is _really_ interesting :)
> > 
> > May be that you have had time to read my last email in reply to a message 
of 
> > Phillip P. Soon after writing of the arguments in favor of using 
> > wait_for_completion_killable() (in patch 2/3 of the series I sent today), 
I 
> > read your patch.
> > 
> > If you are right (and I think you are) I'll have to send a v2 that 
replaces 
> > the killable wait with an uninterruptible one.
> > 
> > Unfortunately I have not the needed experience to decide whether or not 
to 
> > ack your patch, even if I'm strongly tempted to do it.
> > 
> > Let's wait for more experienced people.
> > 
> > Thanks,
> > 
> > Fabio 
> > 
> 
> 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.
> 

@Phil, Correct: the kernel can choose how to respond to signals.
@Martin, Please correct me if I'm missing something in what follows...

> 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. 

No, this confusion is my fault.

I read Martin's patch, but in my mind I exchanged "SIGTERM" with "SIGKILL".

At this moment, without Martin's patch, only SIGTERM is delivered to the 
kthread. This is due to the line "allow_signal(SIGTERM);".

If we try to kill the kthread with "kill -KILL <PID>", nothing happens. 
Instead if we use "kill -TERM <PID>", the kthread terminates.

For what is related to my code, there is no functional changes between using 
the killable or the uninterruptible version (I guess). But for sake of 
consistency, since SIGKILL is not allowed, I should use either 
wait_for_completion_interruptible() (without Martin's patch) or 
wait_for_completion() (with Martin's patch).

However, I re-iterate that, since SIGKILL is not allowed in the current code, 
"kill -KILL <PID>" has no effect at all and the wait is not interruptible 
with my killable version of the wait.

> From that perspective I guess, it is therefore not 'incorrect' as
> such - if indeed we wanted that behaviour.
> 
> That said, killing it would still cause the behaviour Martin mentions -
> I guess we don't want it to be either killable or interruptible based on
> that logic?

Yes, I agree. I should replace the killable version with the uninterruptible 
one.

Thanks,

Fabio

> 
> Regards,
> Phil
> 