linux-kernel - Re: [PATCH] posix-timers: Handle returned errors poperly in [i]timer

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: <87a5luvhix.fsf@somnus>
Date: Mon, 15 Apr 2024 16:15:34 +0200
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, LKML
 <linux-kernel@...r.kernel.org>, Frederic Weisbecker <frederic@...nel.org>,
 John Stultz <jstultz@...gle.com>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...nel.org>, Stephen Boyd <sboyd@...nel.org>, Eric
 Biederman <ebiederm@...ssion.com>
Subject: Re: [PATCH] posix-timers: Handle returned errors poperly in
 [i]timer_delete()

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

> On 04/15, Anna-Maria Behnsen wrote:
>>
>> timer_delete_hook() returns -EINVAL when the clock or the timer_del
>> callback of the clock does not exist. This return value is not handled by
>> the callsites timer_delete() and itimer_delete().
>
> IIUC this shouldn't happen? timer_delete_hook() WARN()s in this case,
> not sure we need to return this error to userspace...

This shouldn't happen, right.

Even if we do not return this error to userspace, is it valid to proceed
with the rest of the callsites? When it is fine to just ignore the
-EINVAL return, then I would propose just to add a comment to the code.

>> --- a/kernel/time/posix-timers.c
>> +++ b/kernel/time/posix-timers.c
>> @@ -1009,6 +1009,7 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
>>  {
>>  	struct k_itimer *timer;
>>  	unsigned long flags;
>> +	int ret;
>>  
>>  	timer = lock_timer(timer_id, &flags);
>>  
>> @@ -1019,7 +1020,11 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
>>  	/* Prevent signal delivery and rearming. */
>>  	timer->it_signal_seq++;
>>  
>> -	if (unlikely(timer_delete_hook(timer) == TIMER_RETRY)) {
>> +	ret = timer_delete_hook(timer);
>> +	if (ret < 0)
>> +		return ret;
>
> unlock_timer() ?
>

bah... was done in a hurry...

Thanks,

	Anna-Maria