Re: [patch V3 17/18] posix-timers: Provide a mechanism to allocate a given timer ID

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, Mar 11, 2025 at 10:35:46PM +0100, Frederic Weisbecker wrote:
> Le Sat, Mar 08, 2025 at 05:48:47PM +0100, Thomas Gleixner a écrit :
> > @@ -364,6 +389,16 @@ static enum hrtimer_restart posix_timer_
> >  	return HRTIMER_NORESTART;
> >  }
> >  
> > +long posixtimer_create_prctl(unsigned long ctrl)
> > +{
> > +	if (ctrl > PR_TIMER_CREATE_RESTORE_IDS_ON)
> > +		return -EINVAL;
> > +
> > +	guard(spinlock_irq)(&current->sighand->siglock);
> > +	current->signal->timer_create_restore_ids = ctrl == PR_TIMER_CREATE_RESTORE_IDS_ON;
> 
> Is the locking necessary here? It's not used on the read side.
> It only makes sense if more flags are to be added later in struct signal and the
> fields write can race.

Actually this is a very subtle moment. The @timer_create_restore_ids is a bit field and
updating them without a lock already lead into hard to catch bugs in the past especially
when we have close bits members such as is_child_subreaper/has_child_subreaper near it.
I thought of fork(clone_vm) calls in multithreaded application where real_parent may
point into our task which is doing prctl but didn't find any problem so far (though
internal feeling says that this is not hot path call and better would be to keep Thomas'
original lock code :-). Anyway, seems to be safe without it.

	Cyrill