Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK()

[Kirill Tkhai]

On 17.01.2018 00:13, Oleg Nesterov wrote:
> On 01/16, Kirill Tkhai wrote:
>>
>> On 15.01.2018 23:51, Oleg Nesterov wrote:
>>>
>>>>  kill:
>>>> -		force_sig(SIGKILL, p);
>>>> +		send_sig(SIGKILL, p, 1);
>>>
>>> Agreed, I didn't actually want to use force_sig(SIGKILL), copy-and-paste error.
>>
>> force_sig() is still safe under tasklist_lock as release_task() unhashes a task
>> from the lists and destroys sighand at the same time under it. So, it seems
>> there is no a problem :)
> 
> I didn't mean it is unsafe. The problem is that force_sig() replaced send_sig()
> to avoid tasklist_lock which we no longer take in send-signal paths. Another
> problem is that it differs from send_sig(SIGKILL) used in other places and this
> difference (ability to kill SIGNAL_UNKILLABLE tasks) was added by accident, that
> was my point.
> 
>> Anyway, we could use send_sig_info(SIGKILL, SEND_SIG_FORCED, p) instead of that
>> in the patch like you suggested below.
> 
> Probably, but this needs another/separate change.

Ok, as there is no single right answer on this problem, let's skip it for this patchset.

>> Also we skip global init on session iteration. This could be useful for debugging,
>> when init is "/bin/bash" and some task started on top of bash is hunged.
> 
> We will need this only after we use SEND_SIG_FORCED, send_sig(SIGKILL) won't kill
> init.
> 
>>> This looks strange, and probably unintentional. So it seems yoou should start
>>> with "revert 20ac94378 [PATCH] do_SAK: Don't recursively take the tasklist_lock" ?
>>> The original reason for that commit has gone a long ago.
>>
>> If we revert it, lock_task_sighand() will be nested with task_lock().
> 
> This is safe. lock_task_sighand() is irq-safe (just like ->siglock) and it
> is actually used in irqs. Thus it is safe to use it under task_lock() which
> doesn't disable irqs.
> 
> And,
> 
>> Yeah, it's not for
>> a long time, next commit will change that.
> 
> Yes, there is no reason to send SIGKILL under task_lock().
> 
>>> At the same time, I do not know if we actually want to kill sub-namespace inits
>>> or not. If yes, we can use SEND_SIG_FORCED (better than ugly force_sig()) but
>>> skip the global init. But this will need yet another change.
>>
>> From https://www.kernel.org/doc/Documentation/SAK.txt:
>>
>> "An operating system's Secure Attention Key is a security tool which is
>>  provided as protection against trojan password capturing programs.  It
>>  is an undefeatable way of killing all programs which could be
>>  masquerading as login applications"
>>
>> It seems, since not privileged user is able to create pid_ns to start
>> a "trojan password capturing program", we have to kill sub-namespace inits too.
> 
> Agreed, that is why I suggested SEND_SIG_FORCED.
> 
> However. this is the user-visible change and who knows, perhaps it is too late
> to change the current behaviour. So I think we should do this after cleanups,
> this way we can easily revert it later in (unlikely) case someone complains.
> 
> But, Kirill, this is up to you, I won't insist.

Thanks, Oleg. I've sent v2 "[PATCH v2 0/3] tty: Make __do_SAK() less greedy in regard to tasklist_lock"
taking into account all of your comments.

(It seems this theme is not interesting for most people. I've removed them from CC in v2,
to reduce people mail traffic)

Thanks,
Kirill