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 for Android: free password hash cracker in your pocket

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

Message-ID: <87fu2apoeh.fsf@xmission.com>
Date:   Tue, 29 May 2018 08:44:22 -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:

> On Tue, May 29, 2018 at 07:24:26AM -0500, Eric W. Biederman wrote:
>> 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.
>
> I do disagree here tbh. The goto jump into it the if part of an if-else
> seems pretty nasty.
> I also don't know why this should be confusing the logic. There's a
> single function that is called in two places and it is declared directly
> atop it's only caller. Additionally, recognizing a single name of a
> function as being the same in two places is way easier then recognizing
> that a multi-line pattern is the same in two places.

But there are not two places.  There is only one place.
The logic might be cleaned up reorganizing the tests a little bit.
Something like this perhaps.

	/*
	 * Collect the siginfo appropriate to this signal.  Check if
	 * there is another siginfo for the same signal.
	*/
	list_for_each_entry(q, &list->list, list) {
		if (q->info.si_signo == sig) {
			if (first)
                        	break;
			first = q;
		}
	}

	/* Not still pending? */
	if (!first || (&q->list != &list->list))
        	sigdelset(&list->signal, sig);
	if (first) {
		...


The logic at a high level is:
	Is there another instance of this signal pending?
             yes?  Then don't "sigdelset"
        Do we have siginfo?
           yes? return it.
           no?  dummy up a siginfo.

Making that logic clearer would be nice.  Obscuring it with
an extra function just obstructs maintenance.

Eric