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

Christian Brauner
2020-03-10T21:22:06Z

On Sun, Mar 08, 2020 at 04:36:17PM -0500, 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>
> ---
> 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);
> }

This is fine for now but we share an aweful lot of code with
copy_sighand(). We should earmark this to look into consolidating the
core operations into a common helper called from both copy_sighand() and
unshare_sighand() maybe even dumbing it down to one helper. But not
needed for now.

Otherwise:
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>