* [PATCH 0/3] staging: r8188eu: clean up osdep_service.h @ 2021-10-16 18:13 Martin Kaiser 2021-10-16 18:13 ` [PATCH 1/3] staging: r8188eu: res_to_status is unused Martin Kaiser ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Martin Kaiser @ 2021-10-16 18:13 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] staging: r8188eu: res_to_status is unused 2021-10-16 18:13 [PATCH 0/3] staging: r8188eu: clean up osdep_service.h Martin Kaiser @ 2021-10-16 18:13 ` Martin Kaiser 2021-10-16 18:54 ` Fabio M. De Francesco 2021-10-17 12:46 ` Michael Straube 2021-10-16 18:13 ` [PATCH 2/3] staging: r8188eu: daemonize is not defined Martin Kaiser 2021-10-16 18:13 ` [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread Martin Kaiser 2 siblings, 2 replies; 18+ messages in thread From: Martin Kaiser @ 2021-10-16 18:13 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser The function res_to_status is not used. Remove it. Signed-off-by: Martin Kaiser <martin@kaiser.cx> --- drivers/staging/r8188eu/include/osdep_service.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h index 4622ce400e41..d148137adf39 100644 --- a/drivers/staging/r8188eu/include/osdep_service.h +++ b/drivers/staging/r8188eu/include/osdep_service.h @@ -174,11 +174,6 @@ static inline void flush_signals_thread(void) flush_signals(current); } -static inline int res_to_status(int res) -{ - return res; -} - #define _RND(sz, r) ((((sz)+((r)-1))/(r))*(r)) #define RND4(x) (((x >> 2) + (((x & 3) == 0) ? 0: 1)) << 2) -- 2.20.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] staging: r8188eu: res_to_status is unused 2021-10-16 18:13 ` [PATCH 1/3] staging: r8188eu: res_to_status is unused Martin Kaiser @ 2021-10-16 18:54 ` Fabio M. De Francesco 2021-10-17 12:46 ` Michael Straube 1 sibling, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-16 18:54 UTC (permalink / raw) To: Greg Kroah-Hartman, Martin Kaiser Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser On Saturday, October 16, 2021 8:13:41 PM CEST Martin Kaiser wrote: > The function res_to_status is not used. Remove it. > > Signed-off-by: Martin Kaiser <martin@kaiser.cx> > --- > drivers/staging/r8188eu/include/osdep_service.h | 5 ----- > 1 file changed, 5 deletions(-) Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> Regards, Fabio ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] staging: r8188eu: res_to_status is unused 2021-10-16 18:13 ` [PATCH 1/3] staging: r8188eu: res_to_status is unused Martin Kaiser 2021-10-16 18:54 ` Fabio M. De Francesco @ 2021-10-17 12:46 ` Michael Straube 1 sibling, 0 replies; 18+ messages in thread From: Michael Straube @ 2021-10-17 12:46 UTC (permalink / raw) To: Martin Kaiser, Greg Kroah-Hartman Cc: Larry Finger, Phillip Potter, linux-staging, linux-kernel On 10/16/21 20:13, Martin Kaiser wrote: > The function res_to_status is not used. Remove it. > > Signed-off-by: Martin Kaiser <martin@kaiser.cx> > --- > drivers/staging/r8188eu/include/osdep_service.h | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h > index 4622ce400e41..d148137adf39 100644 > --- a/drivers/staging/r8188eu/include/osdep_service.h > +++ b/drivers/staging/r8188eu/include/osdep_service.h > @@ -174,11 +174,6 @@ static inline void flush_signals_thread(void) > flush_signals(current); > } > > -static inline int res_to_status(int res) > -{ > - return res; > -} > - > #define _RND(sz, r) ((((sz)+((r)-1))/(r))*(r)) > #define RND4(x) (((x >> 2) + (((x & 3) == 0) ? 0: 1)) << 2) > > Acked-by: Michael Straube <straube.linux@gmail.com> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] staging: r8188eu: daemonize is not defined 2021-10-16 18:13 [PATCH 0/3] staging: r8188eu: clean up osdep_service.h Martin Kaiser 2021-10-16 18:13 ` [PATCH 1/3] staging: r8188eu: res_to_status is unused Martin Kaiser @ 2021-10-16 18:13 ` Martin Kaiser 2021-10-16 18:59 ` Fabio M. De Francesco 2021-10-17 12:48 ` Michael Straube 2021-10-16 18:13 ` [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread Martin Kaiser 2 siblings, 2 replies; 18+ messages in thread From: Martin Kaiser @ 2021-10-16 18:13 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser Remove dead code that depends on daemonize. Signed-off-by: Martin Kaiser <martin@kaiser.cx> --- drivers/staging/r8188eu/include/osdep_service.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h index d148137adf39..ee8a64bb3126 100644 --- a/drivers/staging/r8188eu/include/osdep_service.h +++ b/drivers/staging/r8188eu/include/osdep_service.h @@ -162,9 +162,6 @@ static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer) static __inline void thread_enter(char *name) { -#ifdef daemonize - daemonize("%s", name); -#endif allow_signal(SIGTERM); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] staging: r8188eu: daemonize is not defined 2021-10-16 18:13 ` [PATCH 2/3] staging: r8188eu: daemonize is not defined Martin Kaiser @ 2021-10-16 18:59 ` Fabio M. De Francesco 2021-10-17 12:48 ` Michael Straube 1 sibling, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-16 18:59 UTC (permalink / raw) To: Greg Kroah-Hartman, Martin Kaiser Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser On Saturday, October 16, 2021 8:13:42 PM CEST Martin Kaiser wrote: > Remove dead code that depends on daemonize. > > Signed-off-by: Martin Kaiser <martin@kaiser.cx> > --- > drivers/staging/r8188eu/include/osdep_service.h | 3 --- > 1 file changed, 3 deletions(-) Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> Regards, Fabio ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] staging: r8188eu: daemonize is not defined 2021-10-16 18:13 ` [PATCH 2/3] staging: r8188eu: daemonize is not defined Martin Kaiser 2021-10-16 18:59 ` Fabio M. De Francesco @ 2021-10-17 12:48 ` Michael Straube 1 sibling, 0 replies; 18+ messages in thread From: Michael Straube @ 2021-10-17 12:48 UTC (permalink / raw) To: Martin Kaiser, Greg Kroah-Hartman Cc: Larry Finger, Phillip Potter, linux-staging, linux-kernel On 10/16/21 20:13, Martin Kaiser wrote: > Remove dead code that depends on daemonize. > > Signed-off-by: Martin Kaiser <martin@kaiser.cx> > --- > drivers/staging/r8188eu/include/osdep_service.h | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/staging/r8188eu/include/osdep_service.h b/drivers/staging/r8188eu/include/osdep_service.h > index d148137adf39..ee8a64bb3126 100644 > --- a/drivers/staging/r8188eu/include/osdep_service.h > +++ b/drivers/staging/r8188eu/include/osdep_service.h > @@ -162,9 +162,6 @@ static inline unsigned char _cancel_timer_ex(struct timer_list *ptimer) > > static __inline void thread_enter(char *name) > { > -#ifdef daemonize > - daemonize("%s", name); > -#endif > allow_signal(SIGTERM); > } > > Acked-by: Michael Straube <straube.linux@gmail.com> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-16 18:13 [PATCH 0/3] staging: r8188eu: clean up osdep_service.h Martin Kaiser 2021-10-16 18:13 ` [PATCH 1/3] staging: r8188eu: res_to_status is unused Martin Kaiser 2021-10-16 18:13 ` [PATCH 2/3] staging: r8188eu: daemonize is not defined Martin Kaiser @ 2021-10-16 18:13 ` Martin Kaiser 2021-10-16 18:53 ` Fabio M. De Francesco 2021-10-17 14:47 ` Fabio M. De Francesco 2 siblings, 2 replies; 18+ messages in thread From: Martin Kaiser @ 2021-10-16 18:13 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser 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 <martin@kaiser.cx> --- 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-16 18:13 ` [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread Martin Kaiser @ 2021-10-16 18:53 ` Fabio M. De Francesco 2021-10-17 10:29 ` Phillip Potter 2021-10-17 14:47 ` Fabio M. De Francesco 1 sibling, 1 reply; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-16 18:53 UTC (permalink / raw) To: Greg Kroah-Hartman, Martin Kaiser Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser 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 <martin@kaiser.cx> > --- > 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 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-16 18:53 ` Fabio M. De Francesco @ 2021-10-17 10:29 ` Phillip Potter 2021-10-17 12:51 ` Fabio M. De Francesco ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Phillip Potter @ 2021-10-17 10:29 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Greg Kroah-Hartman, Martin Kaiser, Larry Finger, Michael Straube, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 10:29 ` Phillip Potter @ 2021-10-17 12:51 ` Fabio M. De Francesco 2021-10-17 14:13 ` Fabio M. De Francesco 2021-10-17 18:02 ` Martin Kaiser 2021-10-17 13:14 ` Fabio M. De Francesco 2021-10-17 13:19 ` Fabio M. De Francesco 2 siblings, 2 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-17 12:51 UTC (permalink / raw) To: Phillip Potter Cc: Greg Kroah-Hartman, Martin Kaiser, Larry Finger, Michael Straube, linux-staging, linux-kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 12:51 ` Fabio M. De Francesco @ 2021-10-17 14:13 ` Fabio M. De Francesco 2021-10-17 18:02 ` Martin Kaiser 1 sibling, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-17 14:13 UTC (permalink / raw) To: Phillip Potter Cc: Greg Kroah-Hartman, Martin Kaiser, Larry Finger, Michael Straube, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 12:51 ` Fabio M. De Francesco 2021-10-17 14:13 ` Fabio M. De Francesco @ 2021-10-17 18:02 ` Martin Kaiser 2021-10-17 20:12 ` Phillip Potter 1 sibling, 1 reply; 18+ messages in thread From: Martin Kaiser @ 2021-10-17 18:02 UTC (permalink / raw) To: Fabio M. De Francesco Cc: Phillip Potter, Greg Kroah-Hartman, Larry Finger, Michael Straube, linux-staging, linux-kernel Hi Fabio and all, Thus wrote Fabio M. De Francesco (fmdefrancesco@gmail.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. > > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 18:02 ` Martin Kaiser @ 2021-10-17 20:12 ` Phillip Potter 0 siblings, 0 replies; 18+ messages in thread From: Phillip Potter @ 2021-10-17 20:12 UTC (permalink / raw) To: Martin Kaiser Cc: Fabio M. De Francesco, Greg Kroah-Hartman, Larry Finger, Michael Straube, linux-staging, linux-kernel On Sun, Oct 17, 2021 at 08:02:37PM +0200, Martin Kaiser wrote: > Hi Fabio and all, > > Thus wrote Fabio M. De Francesco (fmdefrancesco@gmail.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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 10:29 ` Phillip Potter 2021-10-17 12:51 ` Fabio M. De Francesco @ 2021-10-17 13:14 ` Fabio M. De Francesco 2021-10-17 14:11 ` Fabio M. De Francesco 2021-10-17 13:19 ` Fabio M. De Francesco 2 siblings, 1 reply; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-17 13:14 UTC (permalink / raw) To: Phillip Potter, Martin Kaiser Cc: Greg Kroah-Hartman, Larry Finger, Michael Straube, linux-staging, linux-kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 13:14 ` Fabio M. De Francesco @ 2021-10-17 14:11 ` Fabio M. De Francesco 0 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-17 14:11 UTC (permalink / raw) To: Phillip Potter, Martin Kaiser Cc: Greg Kroah-Hartman, Larry Finger, Michael Straube, linux-staging, linux-kernel 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... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-17 10:29 ` Phillip Potter 2021-10-17 12:51 ` Fabio M. De Francesco 2021-10-17 13:14 ` Fabio M. De Francesco @ 2021-10-17 13:19 ` Fabio M. De Francesco 2 siblings, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-17 13:19 UTC (permalink / raw) To: Phillip Potter, Martin Kaiser Cc: Greg Kroah-Hartman, Larry Finger, Michael Straube, linux-staging, linux-kernel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread 2021-10-16 18:13 ` [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread Martin Kaiser 2021-10-16 18:53 ` Fabio M. De Francesco @ 2021-10-17 14:47 ` Fabio M. De Francesco 1 sibling, 0 replies; 18+ messages in thread From: Fabio M. De Francesco @ 2021-10-17 14:47 UTC (permalink / raw) To: Greg Kroah-Hartman, Martin Kaiser Cc: Larry Finger, Phillip Potter, Michael Straube, linux-staging, linux-kernel, Martin Kaiser 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 <martin@kaiser.cx> > --- > 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 <fmdefrancesco@gmail.com> Regards, Fabio ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-10-17 20:16 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-16 18:13 [PATCH 0/3] staging: r8188eu: clean up osdep_service.h Martin Kaiser 2021-10-16 18:13 ` [PATCH 1/3] staging: r8188eu: res_to_status is unused Martin Kaiser 2021-10-16 18:54 ` Fabio M. De Francesco 2021-10-17 12:46 ` Michael Straube 2021-10-16 18:13 ` [PATCH 2/3] staging: r8188eu: daemonize is not defined Martin Kaiser 2021-10-16 18:59 ` Fabio M. De Francesco 2021-10-17 12:48 ` Michael Straube 2021-10-16 18:13 ` [PATCH 3/3] staging: r8188eu: don't accept SIGTERM for cmd thread Martin Kaiser 2021-10-16 18:53 ` Fabio M. De Francesco 2021-10-17 10:29 ` Phillip Potter 2021-10-17 12:51 ` Fabio M. De Francesco 2021-10-17 14:13 ` Fabio M. De Francesco 2021-10-17 18:02 ` Martin Kaiser 2021-10-17 20:12 ` Phillip Potter 2021-10-17 13:14 ` Fabio M. De Francesco 2021-10-17 14:11 ` Fabio M. De Francesco 2021-10-17 13:19 ` Fabio M. De Francesco 2021-10-17 14:47 ` Fabio M. De Francesco
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).