Re: [PATCH 1/1] signal: make group kill signal fatal

[Oleg Nesterov <oleg@redhat.com>]

On 05/25, Jiri Slaby wrote:
>
> On 05/25/2009 02:07 AM, Oleg Nesterov wrote:
> > On 05/24, Jiri Slaby wrote:
> >>
> >> __fatal_signal_pending() returns now true only for a non-group sent
> >> sigkill, i. e. for example tgkill, send_sig...
> >
> > No. Please look at complete_signal(). If we queue a fatal signal,
> > we always add SIGKILL to any thread.
>
> Ah, thanks. But it looks like it doesn't work well in some cases.
> Consider this kernel code:
>
> ...
> static int release(struct inode *ino, struct file *file)
> {
>         unsigned int a;
>         printk(KERN_DEBUG "fst\n");
>         for (a = 0; a < 10; a++) {
>                 printk(KERN_DEBUG "%s: SP=%u FSP=%u pend=%.16lx
> shpend=%.16lx\n",
>                                 __func__,
>                                 signal_pending(current),
>                                 fatal_signal_pending(current),
>                                 current->pending.signal.sig[0],
>
> current->signal->shared_pending.signal.sig[0]);
>
> ...
>
> int main(int argc, char **argv)
> {
>         struct pollfd fds = { .events = POLLIN };
>         int fd;
>
>         fd = open("/dev/m", O_RDONLY);
>         if (fd < 0)
>                 err(1, "open");
>         fds.fd = fd;
>         if (poll(&fds, 1, -1) < 0)
>                 err(2, "poll");
>         close(fd);
>         return 0;
> }
>
> ----------------------------------------------
> It outputs:
> fst
> release: SP=1 FSP=0 pend=0000000000000000 shpend=0000000000000100

Because (I guess) ->release() is called by do_exit()->close_files(),
by this time the private SIGKILL is already dequeued.

If you kill this program, poll() never returns to the user-space.

> If the poll isn't there, it works well.

Hmm. this is strange. Do you mean that if this program does
sleep(10000) (or something else) instead of poll() above, it
prints pend != 0 ?



Note. There are known issues with fatal_signal_pending() in exit()
path. But in any case, we should not check ->shared_pending.
And even if ->signal != NULL we must not use ->siglock. Because
schedule() checks fatal_signal_pending() under rq->lock, we can
deadlock. Also, the fact that SIGKILL is actually pending is the
current implementation detail, probably this will be changed.

__fatal_signal_pending() could check SIGNAL_GROUP_EXIT, but we
can't do this now, ->signal can be NULL. Actually signal_group_exit()
is more correct.

And. Why do you need fatal_signal_pending() ? It is special,
should be used by things like wait_event_killable().

Oleg.