Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On 04/23, Oleg wrote:
>On 04/23, weizhenliang wrote:
>>
>> Last time Oleg suggested using SIG_DFL as the third parameter, but its type is 'void (*)(int)', but not expected 'struct k_sigaction *'.
>
>Yes I misread the signature of TRACE_EVENT(signal_deliver), and I thought you at least compiled the kernel with your patch applied ;)
>
>> How about
>> 	trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, &sighand->action[signr 
>> - 1]); ?
>
>sure, this should fix the problem.

Sorry about that, I will pay more attention to it in the future.
And thanks for your reply, I will re-adjust the patch later.

Wei.

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On 04/23, Greg KH wrote:
>On Mon, Apr 22, 2019 at 10:59:50PM +0800, Zhenliang Wei wrote:
>> In the fixes commit, removing SIGKILL from each thread signal mask and 
>> executing "goto fatal" directly will skip the call to 
>> "trace_signal_deliver". At this point, the delivery tracking of the 
>> SIGKILL signal will be inaccurate.
>> 
>> Therefore, we need to add trace_signal_deliver before "goto fatal"
>> after executing sigdelset.
>> 
>> Note: The action[SIGKILL] must be SIG_DFL, and SEND_SIG_NOINFO matches 
>> the fact that SIGKILL doesn't have any info.
>> 
>> Acked-by: Christian Brauner <christian@brauner.io>
>> Fixes: cf43a757fd4944 ("signal: Restore the stop PTRACE_EVENT_EXIT")
>> Signed-off-by: Zhenliang Wei <weizhenliang@huawei.com>
>> ---
>>  kernel/signal.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/kernel/signal.c b/kernel/signal.c index 
>> 227ba170298e..0f69ada376ef 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
>>  	if (signal_group_exit(signal)) {
>>  		ksig->info.si_signo = signr = SIGKILL;
>>  		sigdelset(&current->pending.signal, SIGKILL);
>> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
>>  		recalc_sigpending();
>>  		goto fatal;
>>  	}
>> --
>> 2.14.1.windows.1
>> 
>> 
>
><formletter>
>
>This is not the correct way to submit patches for inclusion in the stable kernel tree.  Please read:
>    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>for how to do this properly.
>
></formletter>

Thans for your reply, is that mean I still need to add tags
	Cc: stable@vger.kernel.org
if I want to submit this patch to the stable kernel tree?

Is my understanding correct or missing?

Wei

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On Tue, Apr 23, 2019 at 09:41 PM Christian wrote:
>On Tue, Apr 23, 2019 at 01:10:52PM +0000, weizhenliang wrote:
>> On Mon, Apr 22, 2019 at 11:25 PM Oleg Nesterov <oleg@redhat.com> wrote:
>> >On 04/22, Zhenliang Wei wrote:
>> >>
>> >> --- a/kernel/signal.c
>> >> +++ b/kernel/signal.c
>> >> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
>> >>  	if (signal_group_exit(signal)) {
>> >>  		ksig->info.si_signo = signr = SIGKILL;
>> >>  		sigdelset(&current->pending.signal, SIGKILL);
>> >> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
>> >>  		recalc_sigpending();
>> >>  		goto fatal;
>> >>  	}
>> >
>> >Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>> 
>> Last time Oleg suggested using SIG_DFL as the third parameter, but its type is 'void (*)(int)', but not expected 'struct k_sigaction *'.
>
>Sigh, I should've caught that in the first commit.
>Although it suggests you didn't even compile your patch...
>
>> 
>> How about
>> 	trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, &sighand->action[signr 
>> - 1]); ?
>
>That should work, yes.

Sorry about that, I was too focused on the functionality and didn't notice the compile warning.
And thanks for your reply, I will pay more attention to it in the future.

Zhenliang Wei

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[weizhenliang]

On Mon, Apr 22, 2019 at 11:25 PM Oleg Nesterov <oleg@redhat.com> wrote:
>On 04/22, Zhenliang Wei wrote:
>>
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
>>  	if (signal_group_exit(signal)) {
>>  		ksig->info.si_signo = signr = SIGKILL;
>>  		sigdelset(&current->pending.signal, SIGKILL);
>> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
>>  		recalc_sigpending();
>>  		goto fatal;
>>  	}
>
>Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Last time Oleg suggested using SIG_DFL as the third parameter, but its type is 'void (*)(int)', but not expected 'struct k_sigaction *'.

How about
	trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, &sighand->action[signr - 1]);
?

Although we know that action[SIGKILL] must be SIG_DFL, in order to adapt to the parameter type, I suggest to use the original variable value.

Zhenliang Wei.

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[Christian Brauner]

On Tue, Apr 23, 2019 at 01:10:52PM +0000, weizhenliang wrote:
> On Mon, Apr 22, 2019 at 11:25 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >On 04/22, Zhenliang Wei wrote:
> >>
> >> --- a/kernel/signal.c
> >> +++ b/kernel/signal.c
> >> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
> >>  	if (signal_group_exit(signal)) {
> >>  		ksig->info.si_signo = signr = SIGKILL;
> >>  		sigdelset(&current->pending.signal, SIGKILL);
> >> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
> >>  		recalc_sigpending();
> >>  		goto fatal;
> >>  	}
> >
> >Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> 
> Last time Oleg suggested using SIG_DFL as the third parameter, but its type is 'void (*)(int)', but not expected 'struct k_sigaction *'.

Sigh, I should've caught that in the first commit.
Although it suggests you didn't even compile your patch...

> 
> How about
> 	trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, &sighand->action[signr - 1]);
> ?

That should work, yes.

Christian

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[Oleg Nesterov]

On 04/23, weizhenliang wrote:
>
> On Mon, Apr 22, 2019 at 11:25 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >On 04/22, Zhenliang Wei wrote:
> >>
> >> --- a/kernel/signal.c
> >> +++ b/kernel/signal.c
> >> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
> >>  	if (signal_group_exit(signal)) {
> >>  		ksig->info.si_signo = signr = SIGKILL;
> >>  		sigdelset(&current->pending.signal, SIGKILL);
> >> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
> >>  		recalc_sigpending();
> >>  		goto fatal;
> >>  	}
> >
> >Reviewed-by: Oleg Nesterov <oleg@redhat.com>
>
> Last time Oleg suggested using SIG_DFL as the third parameter, but its type is 'void (*)(int)', but not expected 'struct k_sigaction *'.

Yes I misread the signature of TRACE_EVENT(signal_deliver), and I thought you at
least compiled the kernel with your patch applied ;)

> How about
> 	trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, &sighand->action[signr - 1]);
> ?

sure, this should fix the problem.

Oleg.

[PATCH v3] signal: trace_signal_deliver when signal_group_exit

[Zhenliang Wei]

In the fixes commit, removing SIGKILL from each thread signal mask
and executing "goto fatal" directly will skip the call to
"trace_signal_deliver". At this point, the delivery tracking of the SIGKILL
signal will be inaccurate.

Therefore, we need to add trace_signal_deliver before "goto fatal"
after executing sigdelset.

Note: The action[SIGKILL] must be SIG_DFL, and SEND_SIG_NOINFO matches
the fact that SIGKILL doesn't have any info.

Acked-by: Christian Brauner <christian@brauner.io>
Fixes: cf43a757fd4944 ("signal: Restore the stop PTRACE_EVENT_EXIT")
Signed-off-by: Zhenliang Wei <weizhenliang@huawei.com>
---
 kernel/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index 227ba170298e..0f69ada376ef 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
 	if (signal_group_exit(signal)) {
 		ksig->info.si_signo = signr = SIGKILL;
 		sigdelset(&current->pending.signal, SIGKILL);
+		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
 		recalc_sigpending();
 		goto fatal;
 	}
-- 
2.14.1.windows.1

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[Oleg Nesterov]

On 04/22, Zhenliang Wei wrote:
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
>  	if (signal_group_exit(signal)) {
>  		ksig->info.si_signo = signr = SIGKILL;
>  		sigdelset(&current->pending.signal, SIGKILL);
> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
>  		recalc_sigpending();
>  		goto fatal;
>  	}

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[Greg KH]

On Mon, Apr 22, 2019 at 10:59:50PM +0800, Zhenliang Wei wrote:
> In the fixes commit, removing SIGKILL from each thread signal mask
> and executing "goto fatal" directly will skip the call to
> "trace_signal_deliver". At this point, the delivery tracking of the SIGKILL
> signal will be inaccurate.
> 
> Therefore, we need to add trace_signal_deliver before "goto fatal"
> after executing sigdelset.
> 
> Note: The action[SIGKILL] must be SIG_DFL, and SEND_SIG_NOINFO matches
> the fact that SIGKILL doesn't have any info.
> 
> Acked-by: Christian Brauner <christian@brauner.io>
> Fixes: cf43a757fd4944 ("signal: Restore the stop PTRACE_EVENT_EXIT")
> Signed-off-by: Zhenliang Wei <weizhenliang@huawei.com>
> ---
>  kernel/signal.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 227ba170298e..0f69ada376ef 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2441,6 +2441,7 @@ bool get_signal(struct ksignal *ksig)
>  	if (signal_group_exit(signal)) {
>  		ksig->info.si_signo = signr = SIGKILL;
>  		sigdelset(&current->pending.signal, SIGKILL);
> +		trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
>  		recalc_sigpending();
>  		goto fatal;
>  	}
> -- 
> 2.14.1.windows.1
> 
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

Hi Zhenliang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.1-rc6 next-20190418]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zhenliang-Wei/signal-trace_signal_deliver-when-signal_group_exit/20190423-062107
config: i386-randconfig-x010-201916 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


All error/warnings (new ones prefixed by >>):

   In file included from arch/x86/include/uapi/asm/signal.h:94:0,
                    from arch/x86/include/asm/signal.h:36,
                    from include/uapi/linux/signal.h:5,
                    from include/linux/signal_types.h:10,
                    from include/linux/sched.h:28,
                    from include/linux/sched/mm.h:7,
                    from kernel/signal.c:16:
   kernel/signal.c: In function 'get_signal':
>> include/uapi/asm-generic/signal-defs.h:24:17: error: passing argument 3 of 'trace_signal_deliver' from incompatible pointer type [-Werror=incompatible-pointer-types]
    #define SIG_DFL ((__force __sighandler_t)0) /* default signal handling */
                    ^
>> kernel/signal.c:2444:50: note: in expansion of macro 'SIG_DFL'
      trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
                                                     ^~~~~~~
   In file included from include/trace/syscall.h:5:0,
                    from include/linux/syscalls.h:86,
                    from kernel/signal.c:29:
   include/linux/tracepoint.h:235:21: note: expected 'struct k_sigaction *' but argument is of type 'void (*)(int)'
     static inline void trace_##name(proto)    \
                        ^
   include/linux/tracepoint.h:398:2: note: in expansion of macro '__DECLARE_TRACE'
     __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),  \
     ^~~~~~~~~~~~~~~
   include/linux/tracepoint.h:534:2: note: in expansion of macro 'DECLARE_TRACE'
     DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
     ^~~~~~~~~~~~~
>> include/trace/events/signal.h:96:1: note: in expansion of macro 'TRACE_EVENT'
    TRACE_EVENT(signal_deliver,
    ^~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/trace_signal_deliver +24 include/uapi/asm-generic/signal-defs.h

b1ecb4c3 include/asm-generic/signal.h Al Viro         2005-05-04  23  
b1ecb4c3 include/asm-generic/signal.h Al Viro         2005-05-04 @24  #define SIG_DFL	((__force __sighandler_t)0)	/* default signal handling */
b1ecb4c3 include/asm-generic/signal.h Al Viro         2005-05-04  25  #define SIG_IGN	((__force __sighandler_t)1)	/* ignore signal */
b1ecb4c3 include/asm-generic/signal.h Al Viro         2005-05-04  26  #define SIG_ERR	((__force __sighandler_t)-1)	/* error return from signal */
b1ecb4c3 include/asm-generic/signal.h Al Viro         2005-05-04  27  #endif
ad158879 include/asm-generic/signal.h David Woodhouse 2006-04-27  28  

:::::: The code at line 24 was first introduced by commit
:::::: b1ecb4c3a9e33cc8b93ac9cb046b535b72a15f68 [PATCH] asm/signal.h unification

:::::: TO: Al Viro <viro@www.linux.org.uk>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36037 bytes --]

Re: [PATCH v3] signal: trace_signal_deliver when signal_group_exit

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]

Hi Zhenliang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.1-rc6 next-20190418]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zhenliang-Wei/signal-trace_signal_deliver-when-signal_group_exit/20190423-062107
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


All errors (new ones prefixed by >>):

   In file included from arch/parisc/include/asm/signal.h:5:0,
                    from include/uapi/linux/signal.h:5,
                    from include/linux/signal_types.h:10,
                    from include/linux/sched.h:28,
                    from include/linux/sched/mm.h:7,
                    from kernel/signal.c:16:
   kernel/signal.c: In function 'get_signal':
>> arch/parisc/include/uapi/asm/signal.h:77:17: error: passing argument 3 of 'trace_signal_deliver' from incompatible pointer type [-Werror=incompatible-pointer-types]
    #define SIG_DFL ((__sighandler_t)0) /* default signal handling */
                    ^
   kernel/signal.c:2444:50: note: in expansion of macro 'SIG_DFL'
      trace_signal_deliver(SIGKILL, SEND_SIG_NOINFO, SIG_DFL);
                                                     ^~~~~~~
   In file included from include/trace/syscall.h:5:0,
                    from include/linux/syscalls.h:86,
                    from kernel/signal.c:29:
   include/linux/tracepoint.h:235:21: note: expected 'struct k_sigaction *' but argument is of type 'void (*)(int)'
     static inline void trace_##name(proto)    \
                        ^
   include/linux/tracepoint.h:398:2: note: in expansion of macro '__DECLARE_TRACE'
     __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),  \
     ^~~~~~~~~~~~~~~
   include/linux/tracepoint.h:534:2: note: in expansion of macro 'DECLARE_TRACE'
     DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
     ^~~~~~~~~~~~~
   include/trace/events/signal.h:96:1: note: in expansion of macro 'TRACE_EVENT'
    TRACE_EVENT(signal_deliver,
    ^~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/trace_signal_deliver +77 arch/parisc/include/uapi/asm/signal.h

70c1674f6 David Howells 2012-10-16  76  
70c1674f6 David Howells 2012-10-16 @77  #define SIG_DFL	((__sighandler_t)0)	/* default signal handling */
70c1674f6 David Howells 2012-10-16  78  #define SIG_IGN	((__sighandler_t)1)	/* ignore signal */
70c1674f6 David Howells 2012-10-16  79  #define SIG_ERR	((__sighandler_t)-1)	/* error return from signal */
70c1674f6 David Howells 2012-10-16  80  

:::::: The code at line 77 was first introduced by commit
:::::: 70c1674f62026e455c0c821fb7f4baf24d2d1139 UAPI: (Scripted) Disintegrate arch/parisc/include/asm

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55418 bytes --]