| Message ID | 20240920000933.185090-2-anjali.k.kulkarni@oracle.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Threads extension for process connector | expand |
On 09/19, Anjali Kulkarni wrote: > > @@ -413,6 +416,10 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, > if (msg->len == sizeof(*pinput)) { > pinput = (struct proc_input *)msg->data; > mc_op = pinput->mcast_op; > + if (mc_op == PROC_CN_MCAST_NOTIFY) { > + current->exit_code = pinput->uexit_code; > + return; ... > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -821,6 +821,7 @@ void __noreturn do_exit(long code) > { > struct task_struct *tsk = current; > int group_dead; > + __u32 uexit_code; > > WARN_ON(irqs_disabled()); > > @@ -863,6 +864,8 @@ void __noreturn do_exit(long code) > tty_audit_exit(); > audit_free(tsk); > > + uexit_code = tsk->exit_code; I don't think you can use task_struct->exit_code. If this task is ptraced, it can be changed/cleared in, say, ptrace_stop() after PROC_CN_MCAST_NOTIFY. Oleg.
> On Sep 20, 2024, at 4:00 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 09/19, Anjali Kulkarni wrote: >> >> @@ -413,6 +416,10 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, >> if (msg->len == sizeof(*pinput)) { >> pinput = (struct proc_input *)msg->data; >> mc_op = pinput->mcast_op; >> + if (mc_op == PROC_CN_MCAST_NOTIFY) { >> + current->exit_code = pinput->uexit_code; >> + return; > > ... > >> --- a/kernel/exit.c >> +++ b/kernel/exit.c >> @@ -821,6 +821,7 @@ void __noreturn do_exit(long code) >> { >> struct task_struct *tsk = current; >> int group_dead; >> + __u32 uexit_code; >> >> WARN_ON(irqs_disabled()); >> >> @@ -863,6 +864,8 @@ void __noreturn do_exit(long code) >> tty_audit_exit(); >> audit_free(tsk); >> >> + uexit_code = tsk->exit_code; > > I don't think you can use task_struct->exit_code. If this task is ptraced, > it can be changed/cleared in, say, ptrace_stop() after PROC_CN_MCAST_NOTIFY. > Thank you, that’s a good point! However, the use case of ptrace, which I assume is for mostly debug and tracing, is exclusive of the use case I am using it for - for production and mostly scaling scenarios. That is, I assume ptrace calls can be done only to your own processes (except superuser), so the tracing process should understand and only do one(ptrace) or the other (request for a exit notification by using a system call) and not both? I could add a comment or something which describes this somewhere. Another point is - if an exit_code is modified, it will anyways be overwritten in the do_exit() call - so it’s not clear to me what the purpose of writing that field would be for ptrace_stop() or any other function…? Is there any other reason for ptrace_stop() to modify task_struct->exit_code? Anjali > Oleg. >
On 09/20, Anjali Kulkarni wrote: > > > On Sep 20, 2024, at 4:00 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > I don't think you can use task_struct->exit_code. If this task is ptraced, > > it can be changed/cleared in, say, ptrace_stop() after PROC_CN_MCAST_NOTIFY. > > > > Thank you, that’s a good point! However, the use case of ptrace, which I assume > is for mostly debug and tracing, is exclusive of the use case I am using it for Well. I don't understand your use-case. Or any other use-case for drivers/connector/ that I know nothing about. But this is irrelevant. The new PROC_CN_MCAST_NOTIFY functionality you propose should work regardless of whether this task is ptraced or not. But it doesn't because the usage of ->exit_code in your patch conflicts with the current usage of this field. So, NACK, sorry. Oleg.
> On Sep 20, 2024, at 11:44 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 09/20, Anjali Kulkarni wrote: >> >>> On Sep 20, 2024, at 4:00 AM, Oleg Nesterov <oleg@redhat.com> wrote: >>> >>> I don't think you can use task_struct->exit_code. If this task is ptraced, >>> it can be changed/cleared in, say, ptrace_stop() after PROC_CN_MCAST_NOTIFY. >>> >> >> Thank you, that’s a good point! However, the use case of ptrace, which I assume >> is for mostly debug and tracing, is exclusive of the use case I am using it for > > Well. I don't understand your use-case. Or any other use-case for drivers/connector/ > that I know nothing about. But this is irrelevant. > > The new PROC_CN_MCAST_NOTIFY functionality you propose should work regardless of Yes, agreed. > whether this task is ptraced or not. But it doesn't because the usage of ->exit_code > in your patch conflicts with the current usage of this field. > Ok, I see that ptrace_stop() seems to be using it in much the same way I want to use it - as a temporary place to store some values. Since in do_exit(), exit_code is overwritten, I didn’t think anyone was using it. Could I add a new field in the task_struct to store my value? (I don’t think there is any other unused/field I can use temporarily) Anjali > So, NACK, sorry. > > Oleg. >
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index 44b19e696176..4c38b9bf4f2f 100644 --- a/drivers/connector/cn_proc.c +++ b/drivers/connector/cn_proc.c @@ -320,7 +320,7 @@ void proc_coredump_connector(struct task_struct *task) send_msg(msg); } -void proc_exit_connector(struct task_struct *task) +void proc_exit_connector(struct task_struct *task, __u32 uexit_code) { struct cn_msg *msg; struct proc_event *ev; @@ -337,7 +337,10 @@ void proc_exit_connector(struct task_struct *task) ev->what = PROC_EVENT_EXIT; ev->event_data.exit.process_pid = task->pid; ev->event_data.exit.process_tgid = task->tgid; - ev->event_data.exit.exit_code = task->exit_code; + if (task->exit_code == 0) + ev->event_data.exit.exit_code = uexit_code; + else + ev->event_data.exit.exit_code = task->exit_code; ev->event_data.exit.exit_signal = task->exit_signal; rcu_read_lock(); @@ -413,6 +416,10 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, if (msg->len == sizeof(*pinput)) { pinput = (struct proc_input *)msg->data; mc_op = pinput->mcast_op; + if (mc_op == PROC_CN_MCAST_NOTIFY) { + current->exit_code = pinput->uexit_code; + return; + } ev_type = pinput->event_type; } else if (msg->len == sizeof(mc_op)) { mc_op = *((enum proc_cn_mcast_op *)msg->data); diff --git a/include/linux/cn_proc.h b/include/linux/cn_proc.h index 1d5b02a96c46..fc1d75897cc7 100644 --- a/include/linux/cn_proc.h +++ b/include/linux/cn_proc.h @@ -27,7 +27,7 @@ void proc_sid_connector(struct task_struct *task); void proc_ptrace_connector(struct task_struct *task, int which_id); void proc_comm_connector(struct task_struct *task); void proc_coredump_connector(struct task_struct *task); -void proc_exit_connector(struct task_struct *task); +void proc_exit_connector(struct task_struct *task, __u32 uexit_code); #else static inline void proc_fork_connector(struct task_struct *task) {} @@ -52,7 +52,8 @@ static inline void proc_ptrace_connector(struct task_struct *task, static inline void proc_coredump_connector(struct task_struct *task) {} -static inline void proc_exit_connector(struct task_struct *task) +static inline void proc_exit_connector(struct task_struct *task, + __u32 uexit_code) {} #endif /* CONFIG_PROC_EVENTS */ #endif /* CN_PROC_H */ diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h index 18e3745b86cd..2b12a24e4651 100644 --- a/include/uapi/linux/cn_proc.h +++ b/include/uapi/linux/cn_proc.h @@ -27,7 +27,8 @@ */ enum proc_cn_mcast_op { PROC_CN_MCAST_LISTEN = 1, - PROC_CN_MCAST_IGNORE = 2 + PROC_CN_MCAST_IGNORE = 2, + PROC_CN_MCAST_NOTIFY = 3 }; #define PROC_EVENT_ALL (PROC_EVENT_FORK | PROC_EVENT_EXEC | PROC_EVENT_UID | \ @@ -65,6 +66,7 @@ enum proc_cn_event { struct proc_input { enum proc_cn_mcast_op mcast_op; enum proc_cn_event event_type; + __u32 uexit_code; }; static inline enum proc_cn_event valid_event(enum proc_cn_event ev_type) diff --git a/kernel/exit.c b/kernel/exit.c index 7430852a8571..e2698ebe59ea 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -821,6 +821,7 @@ void __noreturn do_exit(long code) { struct task_struct *tsk = current; int group_dead; + __u32 uexit_code; WARN_ON(irqs_disabled()); @@ -863,6 +864,8 @@ void __noreturn do_exit(long code) tty_audit_exit(); audit_free(tsk); + uexit_code = tsk->exit_code; + tsk->exit_code = code; taskstats_exit(tsk, group_dead); @@ -900,7 +903,7 @@ void __noreturn do_exit(long code) exit_tasks_rcu_start(); exit_notify(tsk, group_dead); - proc_exit_connector(tsk); + proc_exit_connector(tsk, uexit_code); mpol_put_task_policy(tsk); #ifdef CONFIG_FUTEX if (unlikely(current->pi_state_cache))
Add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a thread to notify the kernel that it has exited abnormally. Thread can also send the exit status code it wants returned in the notification with it. Exiting thread can call this either when it wants to call pthread_exit() with non-zero value or from signal handler. Once kernel receives this, it saves this exit status in the thread's exit_code field of task_struct. This field is then checked when the thread actually exits, and if non-zero, it is copied to the notification to be sent. Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> --- drivers/connector/cn_proc.c | 11 +++++++++-- include/linux/cn_proc.h | 5 +++-- include/uapi/linux/cn_proc.h | 4 +++- kernel/exit.c | 5 ++++- 4 files changed, 19 insertions(+), 6 deletions(-)