Re: [PATCH v2 2/5] exec: Factor unshare_sighand out of de_thread and call it separately

Bernd Edlinger
2020-03-09T19:29:04Z

On 3/8/20 10:36 PM, Eric W. Biederman wrote:
>
> This makes the code clearer and makes it easier to implement a mutex
> that is not taken over any locations that may block indefinitely waiting
> for userspace.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>

Bernd.
> ---
> fs/exec.c | 39 ++++++++++++++++++++++++++-------------
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index c3f34791f2f0..ff74b9a74d34 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1194,6 +1194,23 @@ static int de_thread(struct task_struct *tsk)
> flush_itimer_signals();
> #endif
>
> + BUG_ON(!thread_group_leader(tsk));
> + return 0;
> +
> +killed:
> + /* protects against exit_notify() and __exit_signal() */
> + read_lock(&tasklist_lock);
> + sig->group_exit_task = NULL;
> + sig->notify_count = 0;
> + read_unlock(&tasklist_lock);
> + return -EAGAIN;
> +}
> +
> +
> +static int unshare_sighand(struct task_struct *me)
> +{
> + struct sighand_struct *oldsighand = me->sighand;
> +
> if (refcount_read(&oldsighand->count) != 1) {
> struct sighand_struct *newsighand;
> /*
> @@ -1210,23 +1227,13 @@ static int de_thread(struct task_struct *tsk)
>
> write_lock_irq(&tasklist_lock);
> spin_lock(&oldsighand->siglock);
> - rcu_assign_pointer(tsk->sighand, newsighand);
> + rcu_assign_pointer(me->sighand, newsighand);
> spin_unlock(&oldsighand->siglock);
> write_unlock_irq(&tasklist_lock);
>
> __cleanup_sighand(oldsighand);
> }
> -
> - BUG_ON(!thread_group_leader(tsk));
> return 0;
> -
> -killed:
> - /* protects against exit_notify() and __exit_signal() */
> - read_lock(&tasklist_lock);
> - sig->group_exit_task = NULL;
> - sig->notify_count = 0;
> - read_unlock(&tasklist_lock);
> - return -EAGAIN;
> }
>
> char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> @@ -1264,13 +1271,19 @@ int flush_old_exec(struct linux_binprm * bprm)
> int retval;
>
> /*
> - * Make sure we have a private signal table and that
> - * we are unassociated from the previous thread group.
> + * Make this the only thread in the thread group.
> */
> retval = de_thread(me);
> if (retval)
> goto out;
>
> + /*
> + * Make the signal table private.
> + */
> + retval = unshare_sighand(me);
> + if (retval)
> + goto out;
> +
> /*
> * Must be called _before_ exec_mmap() as bprm->mm is
> * not visibile until then. This also enables the update
>