linux-kernel - Re: [PATCH] signal: Fix the error return of kill -1

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: <87r0o2cs8w.fsf@email.froward.int.ebiederm.org>
Date:   Wed, 16 Aug 2023 21:33:35 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     David Laight <David.Laight@...LAB.COM>,
        Petr Skocik <pskocik@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Marco Elver <elver@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] signal: Fix the error return of kill -1

Oleg Nesterov <oleg@...hat.com> writes:

> On 08/16, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@...hat.com> writes:
>>
>> > On 08/15, David Laight wrote:
>> >>
>> >> or maybe even:
>> >> 	} else {
>> >> 		struct task_struct * p;
>> >> 		int err;
>> >> 		ret = -ESRCH;
>> >>
>> >> 		for_each_process(p) {
>> >> 			if (task_pid_vnr(p) > 1 &&
>> >> 					!same_thread_group(p, current)) {
>> >> 				err = group_send_sig_info(sig, info, p,
>> >> 							  PIDTYPE_MAX);
>> >> 				if (ret)
>> >> 					ret = err;
>> >
>> > Hmm, indeed ;)
>> >
>> > and "err" can be declared inside the loop.
>>
>> We can't remove the success case, from my posted patch.
>>
>> A signal is considered as successfully delivered if at least
>> one process receives it.
>
> Yes.
>
> Initially ret = -ESRCH.
>
> Once group_send_sig_info() succeeds at least once (returns zero)
> ret becomes 0.
>
> After that
>
> 	if (ret)
> 		ret = err;
>
> has no effect.
>
> So if a signal is successfully delivered at least once the code
> above returns zero.

Point.

We should be consistent and ensure  __kill_pgrp_info uses
the same code pattern, otherwise it will be difficult to
see they use the same logic.

Does "if (ret) ret = err;" generate better code than "success |= !err"?

I think for both patterns the reader of the code is going to have to
stop and think about what is going on to understand the logic.

We should probably do something like:

	/* 0 for success or the last error */
	if (ret)
        	ret = err;

I am somewhat partial to keeping the variable "success" simply because
while it's computation is clever it's use in generating the result is
not, so it should be more comprehensible code.  Plus the variable
success seems not to need a comment just a minute to stare at
the code and confirm it works.

Eric