linux-kernel - Re: [PATCH v2 28/37] hrtimer: Implement support for softirq based hrtimers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171110124224.ykr6n4z7zgypojnb@breakpoint.cc>
Date:   Fri, 10 Nov 2017 13:42:24 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Anna-Maria Gleixner <anna-maria@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, keescook@...omium.org,
        Christoph Hellwig <hch@....de>,
        John Stultz <john.stultz@...aro.org>
Subject: Re: [PATCH v2 28/37] hrtimer: Implement support for softirq based
 hrtimers

On 2017-10-22 23:40:06 [+0200], Anna-Maria Gleixner wrote:
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -528,25 +546,42 @@ static ktime_t __hrtimer_next_event_base
>   * Recomputes cpu_base::*next_timer and returns the earliest expires_next but
>   * does not set cpu_base::*expires_next, that is done by hrtimer_reprogram.
>   *
> + * When a softirq is pending, we can ignore the HRTIMER_ACTIVE_SOFT bases,
> + * those timers will get run whenever the softirq gets handled, at the end of
> + * hrtimer_run_softirq(), hrtimer_update_softirq_timer() will re-add these bases.
> + *
> + * Therefore softirq values are those from the HRTIMER_ACTIVE_SOFT clock bases.
> + * The !softirq values are the minima across HRTIMER_ACTIVE, unless an actual
> + * softirq is pending, in which case they're the minima of HRTIMER_ACTIVE_HARD.
> + *
>   * @active_mask must be one of:
>   *  - HRTIMER_ACTIVE,
>   *  - HRTIMER_ACTIVE_SOFT, or
>   *  - HRTIMER_ACTIVE_HARD.
>   */
> -static ktime_t __hrtimer_get_next_event(struct hrtimer_cpu_base *cpu_base,
> -					unsigned int active_mask)
> +static ktime_t
> +__hrtimer_get_next_event(struct hrtimer_cpu_base *cpu_base, unsigned int active_mask)
>  {
>  	unsigned int active;
> +	struct hrtimer *next_timer = NULL;
>  	ktime_t expires_next = KTIME_MAX;
>  
> -	cpu_base->next_timer = NULL;
> +	if (!cpu_base->softirq_activated && (active_mask & HRTIMER_ACTIVE_SOFT)) {
> +		active = cpu_base->active_bases & HRTIMER_ACTIVE_SOFT;
> +		cpu_base->softirq_next_timer = next_timer;
> +		expires_next = __hrtimer_next_event_base(cpu_base, active, expires_next);

doing

               cpu_base->softirq_next_timer = NULL;
               expires_next = __hrtimer_next_event_base(cpu_base, active, KTIME_MAX);

instead would make it more obvious I think. I wasn't sure if it is a
typo and the timer assignment was meant to be after
__hrtimer_next_event_base() was invoked or if NULL was indeed intended.

> +
> +		next_timer = cpu_base->softirq_next_timer;
> +	}
>  
> -	active = cpu_base->active_bases & active_mask;
> -	expires_next = __hrtimer_next_event_base(cpu_base, active, expires_next);
> +	if (active_mask & HRTIMER_ACTIVE_HARD) {
> +		active = cpu_base->active_bases & HRTIMER_ACTIVE_HARD;
> +		cpu_base->next_timer = next_timer;
> +		expires_next = __hrtimer_next_event_base(cpu_base, active, expires_next);
> +	}
>  
>  	return expires_next;
>  }
> -#endif
>  
>  static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
>  {
> @@ -968,6 +1034,32 @@ static inline ktime_t hrtimer_update_low
>  	return tim;
>  }
>  
> +static void
> +hrtimer_update_softirq_timer(struct hrtimer_cpu_base *cpu_base, bool reprogram)
> +{
> +	ktime_t expires;
> +
> +	/*
> +	 * Find the next SOFT expiration.
> +	 */
> +	expires = __hrtimer_get_next_event(cpu_base, HRTIMER_ACTIVE_SOFT);

If you replace the following block
> +
> +	/*
> +	 * reprogramming needs to be triggered, even if the next soft
> +	 * hrtimer expires at the same time than the next hard
> +	 * hrtimer. cpu_base->softirq_expires_next needs to be updated!
> +	 */
> +	if (!reprogram || expires == KTIME_MAX ||
> +	    ktime_before(expires, cpu_base->expires_next))
> +		return;

with 

       if (expires == KTIME_MAX)
               return;
       if (!reprogram || !ktime_before(expires, cpu_base->expires_next)) {

               /*
                * ->softirq_next_timer was updated by __hrtimer_next_event_base()
                * and we need to make sure that ->softirq_expires_next matches.
                */
               cpu_base->softirq_expires_next = expires;
               return;
       }

then you have two bug less I *think*.

If *expires* is before ->expires_next then you don't want to return and
do nothing but instead you want to reprogram timer for the soft-timer
event. 

And then even if *expires* is after ->expires_next then you need to
->softirq_expires_next. As the comment says, the next timer field has
been already updated. At this point, ->softirq_expires_next is set to
KTIME_MAX (due to the raise softirq part) so if this field is not
udpated here, then the hr-irq won't see the pending timer and expire it.
Even worse, if future soft-timer have a "later" expiry time then this
timer now then this field won't be updated at all and all soft-timer
processing will stall.

> +
> +	/*
> +	 * cpu_base->*next_timer is recomputed by __hrtimer_get_next_event()
> +	 * cpu_base->*expires_next is only set by hrtimer_reprogram()
> +	 */
> +	hrtimer_reprogram(cpu_base->softirq_next_timer);
> +}
> +

Sebastian