| Message ID | 20190821092409.13225-1-julien.grall@arm.com |
|---|---|
| Headers | show |
| Series | hrtimer: RT fixes for hrtimer_grab_expiry_lock() | expand |
On Wed, 21 Aug 2019, Julien Grall wrote: > migration_base is used as a placeholder when an hrtimer is switching > between base (see switch_hrtimer_timer_base). It is possible > theoritically possible to have timer->base equal to migration_base. > > Even if it is a placeholder, it would pass all the current check in > hrtimer_grab_expiry_lock() leading to use softirq_expiry_lock > uninitialized. > > This is can be prevented by checking whether the base is equal to > the placeholder (i.e. migration_base). That's a lame argument. The point is that it does not make sense to do that on migration base, but not for the reason you are giving (uninitialized lock). If base == migration_base then there is no point to lock soft_expiry_lock simply because the timer is not executing the callback in soft irq context and the whole lock/unlock dance can be avoided. But, yes. Good catch. Thanks, tglx
Hi Thomas, Thank you for the review. On 21/08/2019 15:02, Thomas Gleixner wrote: > On Wed, 21 Aug 2019, Julien Grall wrote: > >> migration_base is used as a placeholder when an hrtimer is switching >> between base (see switch_hrtimer_timer_base). It is possible >> theoritically possible to have timer->base equal to migration_base. >> >> Even if it is a placeholder, it would pass all the current check in >> hrtimer_grab_expiry_lock() leading to use softirq_expiry_lock >> uninitialized. >> >> This is can be prevented by checking whether the base is equal to >> the placeholder (i.e. migration_base). > > That's a lame argument. The point is that it does not make sense to do that > on migration base, but not for the reason you are giving (uninitialized > lock). Fair point, I will update the commit message. > > If base == migration_base then there is no point to lock soft_expiry_lock > simply because the timer is not executing the callback in soft irq context > and the whole lock/unlock dance can be avoided. > > But, yes. Good catch. Do you want me to resend the series or can I just provide an update to the commit message here? Cheers, -- Julien Grall
Hi all, This small series contains a few fixes for the hrtimer code in RT linux (v5.2.9-rt3-rebase). The patch #2 contains a error I managed to reproduce. The other two are were found while looking at the code. Cheers, Julien Grall (3): hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() hrtimer: Don't grab the expiry lock for non-soft hrtimer hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock() kernel/time/hrtimer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.11.0