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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1957621.GeRc3qvyWe@localhost.localdomain>
Date:   Sun, 17 Oct 2021 14:51:40 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Martin Kaiser <martin@...ser.cx>,
        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.

Correct.

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

I think that kthreads must also allow  SIGKILL with "allow_signal(SIGKILL);" 
for allowing root to make them terminate.

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.

@Martin: Please correct me if I'm wrong. Thanks.

Regards,

Fabio

> 
> 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?
> 
> Regards,
> Phil
>