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().

Therefore add proper error handling.

Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
---
When having a look at the posix timer code during reviewing the queue, I
stumbled over this inconsitency. Maybe you want to have it in your
cleanup queue. Patch applies on top of your queue.

 kernel/time/posix-timers.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

--- 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;
+
+	if (unlikely(ret == TIMER_RETRY)) {
 		/* Unlocks and relocks the timer if it still exists */
 		timer = timer_wait_running(timer, &flags);
 		goto retry_delete;
@@ -1047,6 +1052,7 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
 static void itimer_delete(struct k_itimer *timer)
 {
 	unsigned long flags;
+	int ret;
 
 	/*
 	 * irqsave is required to make timer_wait_running() work.
@@ -1054,13 +1060,17 @@ static void itimer_delete(struct k_itime
 	spin_lock_irqsave(&timer->it_lock, flags);
 
 retry_delete:
+	ret = timer_delete_hook(timer);
+	if (WARN_ON_ONCE(ret < 0))
+		return;
+
 	/*
 	 * Even if the timer is not longer accessible from other tasks
 	 * it still might be armed and queued in the underlying timer
 	 * mechanism. Worse, that timer mechanism might run the expiry
 	 * function concurrently.
 	 */
-	if (timer_delete_hook(timer) == TIMER_RETRY) {
+	if (ret == TIMER_RETRY) {
 		/*
 		 * Timer is expired concurrently, prevent livelocks
 		 * and pointless spinning on RT.

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...

> --- 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() ?

>  static void itimer_delete(struct k_itimer *timer)
>  {
>  	unsigned long flags;
> +	int ret;
>  
>  	/*
>  	 * irqsave is required to make timer_wait_running() work.
> @@ -1054,13 +1060,17 @@ static void itimer_delete(struct k_itime
>  	spin_lock_irqsave(&timer->it_lock, flags);
>  
>  retry_delete:
> +	ret = timer_delete_hook(timer);
> +	if (WARN_ON_ONCE(ret < 0))
> +		return;

the same.

Oleg.

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

Anna-Maria, I can't really answer, I don't understand this code today ;)
That said, let me try to explain my opinion,

On 04/15, Anna-Maria Behnsen wrote:
>
> 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?

Well, I'd say that nothing is safe after we hit the kernel problem.

But lets suppose we return EINVAL and skip list_del(&timer->list)/etc.
How can this help? What can userspace do to resolve this problem? Is it
better to "leak" this timer? I dunno.

> When it is fine to just ignore the
> -EINVAL return, then I would propose just to add a comment to the code.

Agreed!

Oleg.