linux-kernel - Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180115205106.GA30922@redhat.com>
Date:   Mon, 15 Jan 2018 21:51:06 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        jslaby@...e.com, viro@...iv.linux.org.uk, keescook@...omium.org,
        serge@...lyn.com, james.l.morris@...cle.com, luto@...nel.org,
        john.johansen@...onical.com, mingo@...nel.org,
        akpm@...ux-foundation.org, mhocko@...e.com, peterz@...radead.org
Subject: Re: [PATCH 3/4] tty: Iterate only thread group leaders in __do_SAK()

On 01/15, Kirill Tkhai wrote:
>
> On 12.01.2018 19:42, Oleg Nesterov wrote:
>
> > IOW, I do not understand why we can't simply use rcu_read_lock() after
> > do_each_pid_task/while_each_pid_task. Yes we can miss the new process/thread,
> > but if the creator process had this tty opened it should be killed by us so
> > fork/clone can't succeed: both do_fork() and send_sig() take the same lock
> > and do_fork() checks signal_pending() under ->siglock.
> >
> > No?
>
> Yes, but we send signal not every time. So, this was the only reason I added
> lock/unlock the locks. But anyway, __do_SAK() is racy and the effect of that
> is minimal, so it seems we may skip this.

Yes. If we don't send SIGKILL we do not care about the new child process/thread
we can miss, it can't have this tty opened at fork() time. If the child opens
this tty after that, __do_SAK can "miss" it anyway in that it can see it before
it does open(tty).

> I tested your patch with small modification in "struct files_struct *files;" ('*' is added).
> Could I send it with your "Signed-off-by" as the second version?

Yes, please feel free,

>  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.

But. on the second thought this probably needs another change... I don't understand
these force_sig/send_sig in __do_SAK().

If signal->tty == tty it does send_sig(SIGKILL), this won't kill the global or
sub-namespace init.

However, if iterate_fd() finds this tty it does force_sig(SIGKILL) which clears
SIGNAL_UNKILLABLE, so it can kill even the global 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.

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.

Oleg.