On 1/17/22 11:31 AM, Eric W. Biederman wrote:> Mike Christie <michael.christie at oracle.com> writes: > >> On 12/22/21 12:24 PM, Eric W. Biederman wrote: >>> All I am certain of is that you need to set >>> "args->exit_signal = -1;". This prevents having to play games with >>> do_notify_parent. >> >> Hi Eric, >> >> I have all your review comments handled except this one. It's looking like it's >> more difficult than just setting the exit_signal=-1, so I wanted to check that >> I understood you. > > [snip problems with exit_signal = -1] > >> >> What do you think? > > I was wrong. I appear to have confused the thread and the non-thread > cases. > > Perhaps I meant "args->exit_signal = 0". That looks like > do_notify_parent won't send it, and thread_group_leader continues to do > the right thing.That doesn't work too. exit_notify will call do_notify_parent but our parent, qemu, does not ignore SIGCHILD so we will not drop down in into this chunk: psig = tsk->parent->sighand; spin_lock_irqsave(&psig->siglock, flags); if (!tsk->ptrace && sig == SIGCHLD && (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN || (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) { do_notify_parent will return false and so autoreap in exit_notify will be false.> > Baring any additional confusion on my part that cleanly solves the > problem of how not to send a signal from a child process cleanly. > > My apologies for sending you on a wild goose chase. >It's no problem. I learned a lot about signal handling :)
On 1/18/22 12:51 PM, Mike Christie wrote:> On 1/17/22 11:31 AM, Eric W. Biederman wrote: >> Mike Christie <michael.christie at oracle.com> writes: >> >>> On 12/22/21 12:24 PM, Eric W. Biederman wrote: >>>> All I am certain of is that you need to set >>>> "args->exit_signal = -1;". This prevents having to play games with >>>> do_notify_parent. >>> >>> Hi Eric, >>> >>> I have all your review comments handled except this one. It's looking like it's >>> more difficult than just setting the exit_signal=-1, so I wanted to check that >>> I understood you. >> >> [snip problems with exit_signal = -1] >> >>> >>> What do you think? >> >> I was wrong. I appear to have confused the thread and the non-thread >> cases. >> >> Perhaps I meant "args->exit_signal = 0". That looks like >> do_notify_parent won't send it, and thread_group_leader continues to do >> the right thing. > > That doesn't work too. exit_notify will call do_notify_parent but > our parent, qemu, does not ignore SIGCHILD so we will not drop > down in into this chunk: > > psig = tsk->parent->sighand; > spin_lock_irqsave(&psig->siglock, flags); > if (!tsk->ptrace && sig == SIGCHLD && > (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN || > (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) { > > do_notify_parent will return false and so autoreap in exit_notify will > be false. > > > >> >> Baring any additional confusion on my part that cleanly solves the >> problem of how not to send a signal from a child process cleanly. >>Oh yeah, maybe we are thinking about different things. The issue I am hitting is that the parent, qemu, does not know about these task_structs. And, userspace can add/delete these vhost devices dynamically. So qemu could continue to run, but the vhost device could be deleted. In this case I want to free up the task_struct resources since we are no longer using it. With kthreads do_notify_parent returns true and exit_notify calls release_task, so it's handled for me. I had stuck in the USER/VHOST check in the patch you didn't like to get that behavior. If you prefer not adding the VHOST/USER task check in exit_notify then instead of auto reaping, would another possible alternative be to add a modified kernel_wait like function in the vhost layer to reap the task?
Mike Christie <michael.christie at oracle.com> writes:> On 1/17/22 11:31 AM, Eric W. Biederman wrote: >> Mike Christie <michael.christie at oracle.com> writes: >> >>> On 12/22/21 12:24 PM, Eric W. Biederman wrote: >>>> All I am certain of is that you need to set >>>> "args->exit_signal = -1;". This prevents having to play games with >>>> do_notify_parent. >>> >>> Hi Eric, >>> >>> I have all your review comments handled except this one. It's looking like it's >>> more difficult than just setting the exit_signal=-1, so I wanted to check that >>> I understood you. >> >> [snip problems with exit_signal = -1] >> >>> >>> What do you think? >> >> I was wrong. I appear to have confused the thread and the non-thread >> cases. >> >> Perhaps I meant "args->exit_signal = 0". That looks like >> do_notify_parent won't send it, and thread_group_leader continues to do >> the right thing. > > That doesn't work too. exit_notify will call do_notify_parent but > our parent, qemu, does not ignore SIGCHILD so we will not drop > down in into this chunk: > > psig = tsk->parent->sighand; > spin_lock_irqsave(&psig->siglock, flags); > if (!tsk->ptrace && sig == SIGCHLD && > (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN || > (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) { > > do_notify_parent will return false and so autoreap in exit_notify will > be false.Bah good point. We won't send the signal but you won't autoreap either. I think we could legitimately change this bit: /* * Send with __send_signal as si_pid and si_uid are in the * parent's namespaces. */ if (valid_signal(sig) && sig) __send_signal(sig, &info, tsk->parent, PIDTYPE_TGID, false); To add: else /* We don't notify the parent so just autoreap */ autoreap = true; I expect we could make that change all on it's own, it sort of breaks my head that requesting not signaling a parent does not also trigger autoreap behavior. We certainly need some mechanism to trigger autoreap behavior or the technical details of being an extra thread of the process need to be solved. Eric
On 1/18/22 1:12 PM, Eric W. Biederman wrote:> Mike Christie <michael.christie at oracle.com> writes: > >> On 1/17/22 11:31 AM, Eric W. Biederman wrote: >>> Mike Christie <michael.christie at oracle.com> writes: >>> >>>> On 12/22/21 12:24 PM, Eric W. Biederman wrote: >>>>> All I am certain of is that you need to set >>>>> "args->exit_signal = -1;". This prevents having to play games with >>>>> do_notify_parent. >>>> >>>> Hi Eric, >>>> >>>> I have all your review comments handled except this one. It's looking like it's >>>> more difficult than just setting the exit_signal=-1, so I wanted to check that >>>> I understood you. >>> >>> [snip problems with exit_signal = -1] >>> >>>> >>>> What do you think? >>> >>> I was wrong. I appear to have confused the thread and the non-thread >>> cases. >>> >>> Perhaps I meant "args->exit_signal = 0". That looks like >>> do_notify_parent won't send it, and thread_group_leader continues to do >>> the right thing. >> >> That doesn't work too. exit_notify will call do_notify_parent but >> our parent, qemu, does not ignore SIGCHILD so we will not drop >> down in into this chunk: >> >> psig = tsk->parent->sighand; >> spin_lock_irqsave(&psig->siglock, flags); >> if (!tsk->ptrace && sig == SIGCHLD && >> (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN || >> (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) { >> >> do_notify_parent will return false and so autoreap in exit_notify will >> be false. > > Bah good point. We won't send the signal but you won't autoreap either. > > I think we could legitimately change this bit: > > /* > * Send with __send_signal as si_pid and si_uid are in the > * parent's namespaces. > */ > if (valid_signal(sig) && sig) > __send_signal(sig, &info, tsk->parent, PIDTYPE_TGID, false); > > To add: > else > /* We don't notify the parent so just autoreap */ > autoreap = true; >Hey, This works for me, but I think we might have issues where threads now get reaped too soon when they are being ptraced. I think I found a simple solution by just using kernel_wait in the vhost task code since I want to wait for the thread to exit when I'm removing a device already. I posted a patchset so you can check it out.