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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 17 Oct 2021 15:19:28 +0200 From: "Fabio M. De Francesco" <fmdefrancesco@...il.com> To: Phillip Potter <phil@...lpotter.co.uk>, Martin Kaiser <martin@...ser.cx> Cc: 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 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@...t ]# 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 >
Powered by blists - more mailing lists