Re: [PATCH 40/41] ncpfs: Use set_current_blocked()

[Oleg Nesterov]

On 08/17, Petr Vandrovec wrote:
>
> I do not care about 'intr' support, but regular code path blocking all
> signals has to stay.

I do not care about SIGINT/SIGQUIT check, just I simply can't understand
the logic.

But PF_EXITING/SIGKILL code looks absolutely strange.

> PF_EXITING code is there because in the past something was BUG_ON-ing that
> nobody should touch signals while exiting,

Not sure I understand... do you mean we have a kernel bug? it should be
fixed then.

> and same code made sure you
> cannot send signals to exiting process.

Again, can't understand.

A PF_EXITING doesn't need to block the signals at all. See wants_siganl().

But, otoh, even sigblock(SIGKILL) can't protect from SIGKILL or another
fatal signal if this thread has a live subthread.

However, blocking SIGKILL makes the difference if it exits with the pending
SIGKILL. In this case recalc_sigpending() clears TIF_SIGPENDING, this "helps"
to actually sleep in TASK_INTERRUPTIBLE.

At the same time, ncp_do_request() can unblock, say, SIGINT. And this
means that if this task exits because it was killed by SIGINT it won't
block anyway.

And I can't understand why it if fine to kill the live task, but not fine
to interrupt the exiting task. If this was intended. And if you want to
block all signals, why can't you do wait_event_uninterruptible() instead?
Or interruptible/uninterruptible depending on PF_EXITING.


Now suppose it is not PF_EXITING. In this case it is pointless to block
any fatal signal unless this task is single-threaded.


So far I think this code does not understand what it does ;)


OK. I guess nobody cares. Matt, could you redo your original patch?
just remove ->siglock and convert it to use set_current_blocked()
leaving this logic alone.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/