Re: [PATCH v2 1/3] Revert "do_SAK: Don't recursively take the tasklist_lock"

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.com> writes:

> On 01/17, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > On 01/17, Eric W. Biederman wrote:
>> >
>> >> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>> >>
>> >> > This reverts commit 20ac94378de5.
>> >> >
>> >> > send_sig() does not take tasklist_lock for a long time,
>> >> > so this commit and the problem it solves are not relevant
>> >> > anymore.
>> >> >
>> >> > Also, the problem of force_sig() is it clears SIGNAL_UNKILLABLE
>> >> > flag, thus even global init may be killed by __do_SAK(),
>> >> > which is definitely not the expected behavior.
>> >>
>> >> Actually it is.
>> >>
>> >> SAK should kill everything that has the tty open.  If init opens the tty
>> >> I am so sorry, it can not operate correctly.  init should not have your
>> >> tty open.
>> >
>> > OK, but then we need "force" in other places too. __do_SAK() does send_sig(SIGKILL)
>> > in do_each_pid_task(PIDTYPE_SID) and if signal->tty == tty.
>> >
>> > Plus force_sig() is not rcu-friendly.
>> >
>> > So I personally agree with this change. Whether we want to kill the global init
>> > or not should be discussed, if we want to do this __do_SAK() should use
>> > SEND_SIG_FORCED and this is what Kirill is going to do (iiuc), but this needs
>> > another patch.
>>
>> To operate correctly, do_SAK() needs to kill everything that has the tty
>> open.  Unless we can make that guarantee I don't see the point of
>> changing do_SAK.
>
> OK, but how this connects to this change?
>
> Again, this force_sig() doesn't match other send_sig()'s in __do_SAK(),
> and Kirill is going to turn them all into send_sig_info(SEND_SIG_FORCED).
> Just we need to discuss whether we need to skip the global init or not
> but this is another story.
>
> So why do you dislike this change?
>
> force_sig() should die anyway. At least in its current form, it should not
> be used unless task == current. But this is off-topic.

I see that as a fair criticism of force_sig,
and a good argument to use send_sig(SIGKILL, SEND_SIG_FORCED).

Which will kill the global init.

What I don't like is a bunch of patches to introduce races and make
something more racy that should be a logical atomic operation to kill
all of the processes that have a certain tty open so that on the next
open there will be exactly one process with the tty open.

I guess it is a super vhangup.

The purported purpose of SAK is for security.   Breaking security for
performance is not ok.  See what that just did to intel.

So we either need to say do_SAK is broken.  In which case the proper fix
is to just delete the thing.  Or we need not to ensure the final
implemenation is an atomic kill of everything that has the tty open.

I think if these patches can justify using rcu with races in the current
do_SAK implementation than I think do_SAK can just die.  Removing do_SAK
would be a much better way of ensuring do_SAK does not have long lock
hold times.

Races in do_SAK do not justify saying it is ok to introduce more races
in do_SAK.  Either do_SAK is not fit for purpose or it is.

Eric