diff mbox series

[14/45] posix-timers: Consolidate interval retrieval

Message ID 20230606142031.816970056@linutronix.de
State New
Headers
Series posix-timers: Cure inconsistencies and the SIG_IGN mess |

Commit Message

Thomas Gleixner June 6, 2023, 2:37 p.m. UTC 
  There is no point to collect the current interval in the posix clock
specific settime() and gettime() callbacks. Just do it right in the common
code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-cpu-timers.c |   26 ++++++++------------------
 kernel/time/posix-timers.c     |   18 +++++++++---------
 2 files changed, 17 insertions(+), 27 deletions(-)

Comments

Frederic Weisbecker June 28, 2023, 1:08 p.m. UTC | #1 
Le Tue, Jun 06, 2023 at 04:37:40PM +0200, Thomas Gleixner a écrit :
> There is no point to collect the current interval in the posix clock
> specific settime() and gettime() callbacks. Just do it right in the common
> code.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

The only difference I see is that we now return the old interval
even if the target is reaped, which probably doesn't matter anyway.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Thomas Gleixner June 29, 2023, 6:47 p.m. UTC | #2 
On Wed, Jun 28 2023 at 15:08, Frederic Weisbecker wrote:

> Le Tue, Jun 06, 2023 at 04:37:40PM +0200, Thomas Gleixner a écrit :
>> There is no point to collect the current interval in the posix clock
>> specific settime() and gettime() callbacks. Just do it right in the common
>> code.
>> 
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
> The only difference I see is that we now return the old interval
> even if the target is reaped, which probably doesn't matter anyway.

But we don't return it to user space because ret != 0 in that case.
Frederic Weisbecker June 30, 2023, 11:25 a.m. UTC | #3 
On Thu, Jun 29, 2023 at 08:47:30PM +0200, Thomas Gleixner wrote:
> On Wed, Jun 28 2023 at 15:08, Frederic Weisbecker wrote:
> 
> > Le Tue, Jun 06, 2023 at 04:37:40PM +0200, Thomas Gleixner a écrit :
> >> There is no point to collect the current interval in the posix clock
> >> specific settime() and gettime() callbacks. Just do it right in the common
> >> code.
> >> 
> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >
> > The only difference I see is that we now return the old interval
> > even if the target is reaped, which probably doesn't matter anyway.
> 
> But we don't return it to user space because ret != 0 in that case.

In the case of ->set yes but in the case of ->get there is no error
handling.

But still this shouldn't matter as the task is fetched under rcu protection.
And also it can be reaped by the time we return to userspace, so this is
inherently racy. Well, the effect could be visible if current is the reaper
and it has reaped the target already, then it could expect a 0 value in the
interval. I don't think we care though.

Thanks.
Thomas Gleixner June 30, 2023, 1:07 p.m. UTC | #4 
On Fri, Jun 30 2023 at 13:25, Frederic Weisbecker wrote:
> On Thu, Jun 29, 2023 at 08:47:30PM +0200, Thomas Gleixner wrote:
>> On Wed, Jun 28 2023 at 15:08, Frederic Weisbecker wrote:
>> 
>> > Le Tue, Jun 06, 2023 at 04:37:40PM +0200, Thomas Gleixner a écrit :
>> >> There is no point to collect the current interval in the posix clock
>> >> specific settime() and gettime() callbacks. Just do it right in the common
>> >> code.
>> >> 
>> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> >
>> > The only difference I see is that we now return the old interval
>> > even if the target is reaped, which probably doesn't matter anyway.
>> 
>> But we don't return it to user space because ret != 0 in that case.
>
> In the case of ->set yes but in the case of ->get there is no error
> handling.

SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id,
		struct __kernel_itimerspec __user *, setting)
{
	struct itimerspec64 cur_setting;

	int ret = do_timer_gettime(timer_id, &cur_setting);
	if (!ret) {
		if (put_itimerspec64(&cur_setting, setting))

How exactly does this end up being copied to user space if ret != 0?

Thanks,

        tglx
Frederic Weisbecker June 30, 2023, 2:04 p.m. UTC | #5 
On Fri, Jun 30, 2023 at 03:07:17PM +0200, Thomas Gleixner wrote:
> On Fri, Jun 30 2023 at 13:25, Frederic Weisbecker wrote:
> > On Thu, Jun 29, 2023 at 08:47:30PM +0200, Thomas Gleixner wrote:
> >> On Wed, Jun 28 2023 at 15:08, Frederic Weisbecker wrote:
> >> 
> >> > Le Tue, Jun 06, 2023 at 04:37:40PM +0200, Thomas Gleixner a écrit :
> >> >> There is no point to collect the current interval in the posix clock
> >> >> specific settime() and gettime() callbacks. Just do it right in the common
> >> >> code.
> >> >> 
> >> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> >
> >> > The only difference I see is that we now return the old interval
> >> > even if the target is reaped, which probably doesn't matter anyway.
> >> 
> >> But we don't return it to user space because ret != 0 in that case.
> >
> > In the case of ->set yes but in the case of ->get there is no error
> > handling.
> 
> SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id,
> 		struct __kernel_itimerspec __user *, setting)
> {
> 	struct itimerspec64 cur_setting;
> 
> 	int ret = do_timer_gettime(timer_id, &cur_setting);
> 	if (!ret) {
> 		if (put_itimerspec64(&cur_setting, setting))
> 
> How exactly does this end up being copied to user space if ret != 0?

kc->timer_get() doesn't return any value.

So before the patch, interval is retrieved only if the target is not reaped:

timer_gettime() {
    do_timer_gettime() {
        posix_cpu_timer_get() {
            p = cpu_timer_task_rcu(timer);
            if (p)
                itp->interval = ....
        }
    }
}

After the patch it's retrieved unconditionally:

timer_gettime() {
    do_timer_gettime() {
        //unconditionally set
        itp->interval = ....
        posix_cpu_timer_get() {
            p = cpu_timer_task_rcu(timer);
            if (!p)
                //doesn't return any value so no failure reported
        }
    }
}
Thomas Gleixner July 1, 2023, 6:01 p.m. UTC | #6 
On Fri, Jun 30 2023 at 16:04, Frederic Weisbecker wrote:
> On Fri, Jun 30, 2023 at 03:07:17PM +0200, Thomas Gleixner wrote:
>> How exactly does this end up being copied to user space if ret != 0?
>
> kc->timer_get() doesn't return any value.
>
> So before the patch, interval is retrieved only if the target is not reaped:
>
> timer_gettime() {
>     do_timer_gettime() {
>         posix_cpu_timer_get() {
>             p = cpu_timer_task_rcu(timer);
>             if (p)
>                 itp->interval = ....
>         }
>     }
> }
>
> After the patch it's retrieved unconditionally:
>
> timer_gettime() {
>     do_timer_gettime() {
>         //unconditionally set
>         itp->interval = ....
>         posix_cpu_timer_get() {
>             p = cpu_timer_task_rcu(timer);
>             if (!p)
>                 //doesn't return any value so no failure reported

Duh. you are right ...
diff mbox series

Patch

--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -622,8 +622,8 @@  static int posix_cpu_timer_set(struct k_
 {
 	bool sigev_none = timer->it_sigev_notify == SIGEV_NONE;
 	clockid_t clkid = CPUCLOCK_WHICH(timer->it_clock);
-	u64 old_expires, new_expires, old_incr, now;
 	struct cpu_timer *ctmr = &timer->it.cpu;
+	u64 old_expires, new_expires, now;
 	struct sighand_struct *sighand;
 	struct task_struct *p;
 	unsigned long flags;
@@ -660,10 +660,7 @@  static int posix_cpu_timer_set(struct k_
 		return -ESRCH;
 	}
 
-	/*
-	 * Disarm any old timer after extracting its expiry time.
-	 */
-	old_incr = timer->it_interval;
+	/* Retrieve the current expiry time before disarming the timer */
 	old_expires = cpu_timer_getexpires(ctmr);
 
 	if (unlikely(timer->it.cpu.firing)) {
@@ -737,9 +734,6 @@  static int posix_cpu_timer_set(struct k_
 		cpu_timer_fire(timer);
 out:
 	rcu_read_unlock();
-	if (old)
-		old->it_interval = ns_to_timespec64(old_incr);
-
 	return ret;
 }
 
@@ -785,17 +779,13 @@  static void posix_cpu_timer_get(struct k
 
 	rcu_read_lock();
 	p = cpu_timer_task_rcu(timer);
-	if (p) {
-		itp->it_interval = ktime_to_timespec64(timer->it_interval);
-
-		if (cpu_timer_getexpires(ctmr)) {
-			if (CPUCLOCK_PERTHREAD(timer->it_clock))
-				now = cpu_clock_sample(clkid, p);
-			else
-				now = cpu_clock_sample_group(clkid, p, false);
+	if (p && cpu_timer_getexpires(ctmr)) {
+		if (CPUCLOCK_PERTHREAD(timer->it_clock))
+			now = cpu_clock_sample(clkid, p);
+		else
+			now = cpu_clock_sample_group(clkid, p, false);
 
-			__posix_cpu_timer_get(timer, itp, now);
-		}
+		__posix_cpu_timer_get(timer, itp, now);
 	}
 	rcu_read_unlock();
 }
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -636,17 +636,12 @@  static s64 common_hrtimer_forward(struct
  */
 void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 {
+	bool sig_none = timr->it_sigev_notify == SIGEV_NONE;
 	const struct k_clock *kc = timr->kclock;
-	ktime_t now, remaining, iv;
-	bool sig_none;
+	bool iv = !!timr->it_interval;
+	ktime_t now, remaining;
 
-	sig_none = timr->it_sigev_notify == SIGEV_NONE;
-	iv = timr->it_interval;
-
-	/* interval timer ? */
-	if (iv) {
-		cur_setting->it_interval = ktime_to_timespec64(iv);
-	} else if (!timr->it_active) {
+	if (!iv && !timr->it_active) {
 		/*
 		 * SIGEV_NONE oneshot timers are never queued and therefore
 		 * timr->it_active is always false. The check below
@@ -705,6 +700,8 @@  static int do_timer_gettime(timer_t time
 		return -EINVAL;
 
 	memset(setting, 0, sizeof(*setting));
+	setting->it_interval = ktime_to_timespec64(timr->it_interval);
+
 	kc = timr->kclock;
 	if (WARN_ON_ONCE(!kc || !kc->timer_get))
 		ret = -EINVAL;
@@ -918,6 +915,9 @@  static int do_timer_settime(timer_t time
 	if (!timr)
 		return -EINVAL;
 
+	if (old_spec64)
+		old_spec64->it_interval = ktime_to_timespec64(timr->it_interval);
+
 	kc = timr->kclock;
 	if (WARN_ON_ONCE(!kc || !kc->timer_set))
 		error = -EINVAL;