Re: [RFC] connector: add group_exit_code and signal_flags fields to exit_proc_event

[Stefan Strogin]

Hi Evgeniy,

On 09/04/18 02:49, Evgeniy Polyakov wrote:
> Hi everyone
> 
> Sorry for that late reply
> 
> 01.03.2018, 21:58, "Stefan Strogin" :
>> So I was thinking to add these two fields to union event_data:
>> task->signal->group_exit_code
>> task->signal->flags
>> This won't increase size of struct proc_event (because of comm_proc_event)
>> and shouldn't break backward compatibility for the user-space. But it will
>> add some useful information about what caused the process death.
>> What do you think, is it an acceptable approach?
> 
> As I saw in other discussion, doesn't it break userspace API, or you are sure 
> that no sizes has been increased?
> You are using the same structure as used for plain signals and add group 
> status there, how will userspace react,
> if it was compiled with older headers? What if it uses zero-field alignment, 
> i.e. allocating exactly the size of structure with byte precision?
> 

Please ignore this RFC, I was wrong about the fields I need for the problem.
I have sent this patch: https://lkml.org/lkml/2018/3/29/531,
would be grateful for a review.

As for breaking UAPI and structure sizes, look:

> struct proc_event {
...
>   union { /* must be last field of proc_event struct */
...
>   struct comm_proc_event {
>   __kernel_pid_t process_pid;
>   __kernel_pid_t process_tgid;
>   char   comm[16];
>   } comm;
__kernel_pid_t is int that is always 4 bytes in Linux, then
sizeof(event_data.comm) == 24 on all platforms.
> 
>   struct coredump_proc_event {
>   __kernel_pid_t process_pid;
>   __kernel_pid_t process_tgid;
> + __kernel_pid_t parent_pid;
> + __kernel_pid_t parent_tgid;
>   } coredump;
sizeof(event_data.coredump) == 16
> 
>   struct exit_proc_event {
>   __kernel_pid_t process_pid;
>   __kernel_pid_t process_tgid;
>   __u32 exit_code, exit_signal;
> + __kernel_pid_t parent_pid;
> + __kernel_pid_t parent_tgid;
>   } exit;
sizeof(event_data.exit) == 24
> 
>   } event_data;
> };

Therefore, sizeof(event_data) is always = 24 - with old headers and new ones.
sizeof(struct proc_event) is the same as well.
Hence user-space software with old and new headers will allocate the same size.

If the user-space program somehow allocates space only for an internal 
structure,
e.g. for event_data.exit, I still don't see any problems if it allocates and
handles only first 16 bytes of the structure using old headers.

--
Stefan

Re: [RFC] connector: add group_exit_code and signal_flags fields to exit_proc_event

[Evgeniy Polyakov]

Hi everyone

Sorry for that late reply

01.03.2018, 21:58, "Stefan Strogin" :
> So I was thinking to add these two fields to union event_data:
> task->signal->group_exit_code
> task->signal->flags
> This won't increase size of struct proc_event (because of comm_proc_event)
> and shouldn't break backward compatibility for the user-space. But it will
> add some useful information about what caused the process death.
> What do you think, is it an acceptable approach?

As I saw in other discussion, doesn't it break userspace API, or you are sure 
that no sizes has been increased?
You are using the same structure as used for plain signals and add group status 
there, how will userspace react,
if it was compiled with older headers? What if it uses zero-field alignment, 
i.e. allocating exactly the size of structure with byte precision?

[RFC] connector: add group_exit_code and signal_flags fields to exit_proc_event

[Stefan Strogin]

Hello everyone,

I'm working on a user-space application that should handle events when some
processes (children of a certain process, "App1") are being killed with
SIGKILL. Also these notifications must be filtered somehow, e.g. we don't
need to handle cases when exit() generates SIGKILL to child threads
(group exit), for example an OOM-caused SIGKILL and a group exit-generated one
are totally different and we must somehow distinguish them.
Unfortunately we also cannot use SIGCHLD and waitid() in App1, there
should be another application that monitors children of App1.

My idea is to use PROC_EVENT_EXIT. But unfortunately it seems there is not
enough information in exit_proc_event structure to see if that was group
exit-generated SIGKILL or not. Tell me please if I'm wrong.

So I was thinking to add these two fields to union event_data:
task->signal->group_exit_code
task->signal->flags
This won't increase size of struct proc_event (because of comm_proc_event)
and shouldn't break backward compatibility for the user-space. But it will
add some useful information about what caused the process death.
What do you think, is it an acceptable approach?

Thanks

Signed-off-by: Stefan Strogin 
---
 drivers/connector/cn_proc.c  | 2 ++
 include/uapi/linux/cn_proc.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index a782ce87715c..1006cb506a52 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -288,6 +288,8 @@ void proc_exit_connector(struct task_struct *task)
ev->event_data.exit.process_tgid = task->tgid;
ev->event_data.exit.exit_code = task->exit_code;
ev->event_data.exit.exit_signal = task->exit_signal;
+   ev->event_data.exit.group_exit_code = task->signal->group_exit_code;
+   ev->event_data.exit.signal_flags = task->signal->flags;
 
memcpy(>id, _proc_event_id, sizeof(msg->id));
msg->ack = 0; /* not used */
diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h
index 68ff25414700..edf50f398116 100644
--- a/include/uapi/linux/cn_proc.h
+++ b/include/uapi/linux/cn_proc.h
@@ -122,6 +122,8 @@ struct proc_event {
__kernel_pid_t process_pid;
__kernel_pid_t process_tgid;
__u32 exit_code, exit_signal;
+   int group_exit_code;
+   unsigned int signal_flags;
} exit;
 
} event_data;
-- 
2.16.1