Remove unused code. Don't allow user space to kill our cmd thread.
This should be applied on top of "staging: r8188eu: clean up the
Makefile".
Martin Kaiser (3):
staging: r8188eu: res_to_status is unused
staging: r8188eu: daemonize is not defined
staging: r8188eu: don't accept SIGTERM for cmd thread
drivers/staging/r8188eu/core/rtw_cmd.c | 2 --
drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
2 files changed, 15 deletions(-)
--
2.20.1
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.
Signed-off-by: Martin Kaiser <[email protected]>
---
drivers/staging/r8188eu/core/rtw_cmd.c | 2 --
drivers/staging/r8188eu/include/osdep_service.h | 5 -----
2 files changed, 7 deletions(-)
diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/r8188eu/core/rtw_cmd.c
index e17332677daa..b834fac41627 100644
--- a/drivers/staging/r8188eu/core/rtw_cmd.c
+++ b/drivers/staging/r8188eu/core/rtw_cmd.c
@@ -243,8 +243,6 @@ int rtw_cmd_thread(void *context)
struct adapter *padapter = (struct adapter *)context;
struct cmd_priv *pcmdpriv = &padapter->cmdpriv;
- thread_enter("RTW_CMD_THREAD");
-
pcmdbuf = pcmdpriv->cmd_buf;
pcmdpriv->cmdthd_running = true;
diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h
index ee8a64bb3126..886a1b6f30b4 100644
--- a/drivers/staging/r8188eu/include/osdep_service.h
+++ b/drivers/staging/r8188eu/include/osdep_service.h
@@ -160,11 +160,6 @@ static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer)
return del_timer_sync(ptimer);
}
-static __inline void thread_enter(char *name)
-{
- allow_signal(SIGTERM);
-}
-
static inline void flush_signals_thread(void)
{
if (signal_pending (current))
--
2.20.1
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
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/core/rtw_cmd.c | 2 --
> drivers/staging/r8188eu/include/osdep_service.h | 5 -----
> 2 files changed, 7 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_cmd.c b/drivers/staging/
r8188eu/core/rtw_cmd.c
> index e17332677daa..b834fac41627 100644
> --- a/drivers/staging/r8188eu/core/rtw_cmd.c
> +++ b/drivers/staging/r8188eu/core/rtw_cmd.c
> @@ -243,8 +243,6 @@ int rtw_cmd_thread(void *context)
> struct adapter *padapter = (struct adapter *)context;
> struct cmd_priv *pcmdpriv = &padapter->cmdpriv;
>
> - thread_enter("RTW_CMD_THREAD");
> -
> pcmdbuf = pcmdpriv->cmd_buf;
>
> pcmdpriv->cmdthd_running = true;
> diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/
staging/r8188eu/include/osdep_service.h
> index ee8a64bb3126..886a1b6f30b4 100644
> --- a/drivers/staging/r8188eu/include/osdep_service.h
> +++ b/drivers/staging/r8188eu/include/osdep_service.h
> @@ -160,11 +160,6 @@ static inline unsigned char _cancel_timer_ex(struct
timer_list *ptimer)
> return del_timer_sync(ptimer);
> }
>
> -static __inline void thread_enter(char *name)
> -{
> - allow_signal(SIGTERM);
> -}
> -
> static inline void flush_signals_thread(void)
> {
> if (signal_pending (current))
> --
> 2.20.1
>
>
>
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.
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.
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
On Sunday, October 17, 2021 2:51:40 PM CEST Fabio M. De Francesco wrote:
> On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
>
>
> Correct.
Please disregard this second mail. It is a duplicate due to some weird KMail
crashes.
Fabio
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.
>
> Signed-off-by: Martin Kaiser <[email protected]>
> ---
> drivers/staging/r8188eu/core/rtw_cmd.c | 2 --
> drivers/staging/r8188eu/include/osdep_service.h | 5 -----
> 2 files changed, 7 deletions(-)
Dear Martin,
After thinking a little more about this topic, now I'm ready for...
Acked-by: Fabio M. De Francesco <[email protected]>
Regards,
Fabio
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.
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
>
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.
@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
>
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
>
Hi Fabio and all,
Thus wrote Fabio M. De Francesco ([email protected]):
> 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.
> > 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...
Best regards,
Martin
On Sun, Oct 17, 2021 at 08:02:37PM +0200, Martin Kaiser wrote:
> Hi Fabio and all,
>
> Thus wrote Fabio M. De Francesco ([email protected]):
>
> > 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
On Sunday, October 17, 2021 3:14:49 PM CEST Fabio M. De Francesco wrote:
> On Sunday, October 17, 2021 12:29:02 PM CEST Phillip Potter wrote:
Please disregard this email. It's a duplicate due to some weird crashes of
KMail.
Fabio
> @Phil, Correct.
> @Martin, Please correct me if I'm missing something in what follows...