linux-kernel - Re: [PATCH v1 04/20] signal: add copy

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]

Date:   Tue, 29 May 2018 07:24:26 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Christian Brauner <christian@...uner.io>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        mingo@...nel.org, james.morris@...rosoft.com,
        keescook@...omium.org, peterz@...radead.org, sds@...ho.nsa.gov,
        viro@...iv.linux.org.uk, akpm@...ux-foundation.org, oleg@...hat.com
Subject: Re: [PATCH v1 04/20] signal: add copy_pending() helper

Christian Brauner <christian@...uner.io> writes:

> Instead of using a goto for this let's add a simple helper copy_pending()
> which can be called in both places.

Ick no.  As far as I can see this just confuses the logic of the
collect_signal function.

Instead of having two cases with an optional
"sigdelset(&list->signal, sig)" if the signal is no longer in the queue,
you are moving the core work of collect_signal into another function.

At the very least this is going to make maintenance more difficult
as now the work of this function is split into two functions.

It most definitely would have made the bug fix to add resched_timer more
difficult.

Eric

> Signed-off-by: Christian Brauner <christian@...uner.io>
> ---
> v0->v1:
> * patch unchanged
> ---
>  kernel/signal.c | 54 +++++++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index bc750fb4ddcc..baae137455eb 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -515,6 +515,19 @@ int unhandled_signal(struct task_struct *tsk, int sig)
>  	return !tsk->ptrace;
>  }
>  
> +static void copy_pending(siginfo_t *info, struct sigqueue *first,
> +			 bool *resched_timer)
> +{
> +	list_del_init(&first->list);
> +	copy_siginfo(info, &first->info);
> +
> +	*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
> +			 (info->si_code == SI_TIMER) &&
> +			 (info->si_sys_private);
> +
> +	__sigqueue_free(first);
> +}
> +
>  static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
>  			   bool *resched_timer)
>  {
> @@ -526,8 +539,10 @@ static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
>  	*/
>  	list_for_each_entry(q, &list->list, list) {
>  		if (q->info.si_signo == sig) {
> -			if (first)
> -				goto still_pending;
> +			if (first) {
> +				copy_pending(info, first, resched_timer);
> +				return;
> +			}
>  			first = q;
>  		}
>  	}
> @@ -535,29 +550,20 @@ static void collect_signal(int sig, struct sigpending *list, siginfo_t *info,
>  	sigdelset(&list->signal, sig);
>  
>  	if (first) {
> -still_pending:
> -		list_del_init(&first->list);
> -		copy_siginfo(info, &first->info);
> -
> -		*resched_timer =
> -			(first->flags & SIGQUEUE_PREALLOC) &&
> -			(info->si_code == SI_TIMER) &&
> -			(info->si_sys_private);
> -
> -		__sigqueue_free(first);
> -	} else {
> -		/*
> -		 * Ok, it wasn't in the queue.  This must be
> -		 * a fast-pathed signal or we must have been
> -		 * out of queue space.  So zero out the info.
> -		 */
> -		clear_siginfo(info);
> -		info->si_signo = sig;
> -		info->si_errno = 0;
> -		info->si_code = SI_USER;
> -		info->si_pid = 0;
> -		info->si_uid = 0;
> +		copy_pending(info, first, resched_timer);
> +		return;
>  	}
> +
> +	/*
> +	 * Ok, it wasn't in the queue. This must be a fast-pathed signal or we
> +	 * must have been out of queue space. So zero out the info.
> +	 */
> +	clear_siginfo(info);
> +	info->si_signo = sig;
> +	info->si_errno = 0;
> +	info->si_code = SI_USER;
> +	info->si_pid = 0;
> +	info->si_uid = 0;
>  }
>  
>  static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,