Received-SPF: pass (google.com: domain of linux-kernel+bounces-145328-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Anna-Maria Behnsen <anna-maria@linutronix.de>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, LKML
 <linux-kernel@vger.kernel.org>, Frederic Weisbecker <frederic@kernel.org>,
 John Stultz <jstultz@google.com>, Peter Zijlstra <peterz@infradead.org>,
 Ingo Molnar <mingo@kernel.org>, Stephen Boyd <sboyd@kernel.org>, Eric
 Biederman <ebiederm@xmission.com>
Subject: Re: [PATCH] posix-timers: Handle returned errors poperly in
 [i]timer_delete()
In-Reply-To: <20240415130023.GA27124@redhat.com>
Date: Mon, 15 Apr 2024 16:15:34 +0200
Message-ID: <87a5luvhix.fsf@somnus>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Oleg Nesterov <oleg@redhat.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