A timer is visible only when both timer->signal and timer->i_id
are set to their final values.

We can initialize timer->it_id sooner.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 kernel/time/posix-timers.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 204a351a2fd3..1f73ea955756 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -114,6 +114,7 @@ static int posix_timer_add(struct k_itimer *timer)
 
 		spin_lock(&hash_lock);
 		if (!__posix_timers_find(head, sig, id)) {
+			timer->it_id = (timer_t)id;
 			hlist_add_head_rcu(&timer->t_hash, head);
 			spin_unlock(&hash_lock);
 			return id;
@@ -407,8 +408,7 @@ static int do_timer_create(clockid_t which_clock, struct sigevent *event,
 
 	/*
 	 * Add the timer to the hash table. The timer is not yet valid
-	 * because new_timer::it_signal is still NULL. The timer id is also
-	 * not yet visible to user space.
+	 * because new_timer::it_signal is still NULL.
 	 */
 	new_timer_id = posix_timer_add(new_timer);
 	if (new_timer_id < 0) {
@@ -416,7 +416,6 @@ static int do_timer_create(clockid_t which_clock, struct sigevent *event,
 		return new_timer_id;
 	}
 
-	new_timer->it_id = (timer_t) new_timer_id;
 	new_timer->it_clock = which_clock;
 	new_timer->kclock = kc;
 	new_timer->it_overrun = -1LL;
-- 
2.48.1.601.g30ceb7b040-goog

On Wed, Feb 19 2025 at 12:55, Eric Dumazet wrote:
> A timer is visible only when both timer->signal and timer->i_id
> are set to their final values.
>
> We can initialize timer->it_id sooner.

This changelog tells nothing. What is this fixing, why is it required to
initialize this sooner?

We can... We also can not ... Aside of that please write changelogs in
imperative mood as Documentation asks for.

> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  kernel/time/posix-timers.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 204a351a2fd3..1f73ea955756 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -114,6 +114,7 @@ static int posix_timer_add(struct k_itimer *timer)
>  
>  		spin_lock(&hash_lock);
>  		if (!__posix_timers_find(head, sig, id)) {
> +			timer->it_id = (timer_t)id;
>  			hlist_add_head_rcu(&timer->t_hash, head);
>  			spin_unlock(&hash_lock);
>  			return id;
> @@ -407,8 +408,7 @@ static int do_timer_create(clockid_t which_clock, struct sigevent *event,
>  
>  	/*
>  	 * Add the timer to the hash table. The timer is not yet valid
> -	 * because new_timer::it_signal is still NULL. The timer id is also
> -	 * not yet visible to user space.

Even with this change the timer ID is not yet visible to user space
because it has not yet been copied back ....

Thanks,

        tglx

On Thu, Feb 20, 2025 at 9:12 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, Feb 19 2025 at 12:55, Eric Dumazet wrote:
> > A timer is visible only when both timer->signal and timer->i_id
> > are set to their final values.
> >
> > We can initialize timer->it_id sooner.
>
> This changelog tells nothing. What is this fixing, why is it required to
> initialize this sooner?


Just  to make the series more readable and bisectable.

>
> We can... We also can not ... Aside of that please write changelogs in
> imperative mood as Documentation asks for.

I do not understand.

I wrote more than 5000 upstream linux patches, you  are trying to
educate me in 2025 ?

>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  kernel/time/posix-timers.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> > index 204a351a2fd3..1f73ea955756 100644
> > --- a/kernel/time/posix-timers.c
> > +++ b/kernel/time/posix-timers.c
> > @@ -114,6 +114,7 @@ static int posix_timer_add(struct k_itimer *timer)
> >
> >               spin_lock(&hash_lock);
> >               if (!__posix_timers_find(head, sig, id)) {
> > +                     timer->it_id = (timer_t)id;
> >                       hlist_add_head_rcu(&timer->t_hash, head);
> >                       spin_unlock(&hash_lock);
> >                       return id;
> > @@ -407,8 +408,7 @@ static int do_timer_create(clockid_t which_clock, struct sigevent *event,
> >
> >       /*
> >        * Add the timer to the hash table. The timer is not yet valid
> > -      * because new_timer::it_signal is still NULL. The timer id is also
> > -      * not yet visible to user space.
>
> Even with this change the timer ID is not yet visible to user space
> because it has not yet been copied back ....
>

It is seen in concurrent lookups.

On Thu, Feb 20 2025 at 09:48, Eric Dumazet wrote:
> On Thu, Feb 20, 2025 at 9:12 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> On Wed, Feb 19 2025 at 12:55, Eric Dumazet wrote:
>> > A timer is visible only when both timer->signal and timer->i_id
>> > are set to their final values.
>> >
>> > We can initialize timer->it_id sooner.
>>
>> This changelog tells nothing. What is this fixing, why is it required to
>> initialize this sooner?
>
> Just  to make the series more readable and bisectable.

That's fine. But this changelog does not explain what the patch is about.

>> We can... We also can not ... Aside of that please write changelogs in
>> imperative mood as Documentation asks for.
>
> I do not understand.

What's so hard to understand?

What does "We can initialize timer->it_id sooner" tell me?

Yes, we can initialize it sooner, but what's the point? What's the
problem this is solving?

Also if you can't find the relevant documentation about imperative mood,
let me cite it to you:

 "Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
  instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to
  do frotz”, as if you are giving orders to the codebase to change its
  behaviour."

> I wrote more than 5000 upstream linux patches, you  are trying to
> educate me in 2025 ?

No. I'm asking you to provide proper change logs which match what's
requested in documentation. I'm asking that from anyone independent of
patch count.

>> >       /*
>> >        * Add the timer to the hash table. The timer is not yet valid
>> > -      * because new_timer::it_signal is still NULL. The timer id is also
>> > -      * not yet visible to user space.
>>
>> Even with this change the timer ID is not yet visible to user space
>> because it has not yet been copied back ....
>>
> It is seen in concurrent lookups.

But still user space cannot observe it, right?

Thanks,

        tglx