linux-kernel - Re: [RFC][PATCH 4/4] time: Do leapsecond adjustment in gettime fastpaths

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: <20150531160539.GA2497@localhost.localdomain>
Date:	Sun, 31 May 2015 18:05:39 +0200
From:	Richard Cochran <richardcochran@...il.com>
To:	John Stultz <john.stultz@...aro.org>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	Prarit Bhargava <prarit@...hat.com>,
	Daniel Bristot de Oliveira <bristot@...hat.com>,
	Jan Kara <jack@...e.cz>, Jiri Bohac <jbohac@...e.cz>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [RFC][PATCH 4/4] time: Do leapsecond adjustment in gettime
 fastpaths

On Fri, May 29, 2015 at 01:24:28PM -0700, John Stultz wrote:
> Apologies to Richard Cochran, who pushed for such a change
> years ago, which I resisted due to the concerns about the
> performance overhead.

For the record, I got the idea from Michel Hack of IBM.

> While I suspect this isn't extremely critical, folks who
> care about strict leap-second correctness will likely
> want to watch this, and it will likely be a -stable candidate.

I think this is a step in the right direction.  If the 'next_leap_sec'
is made available to the vdso, then the 1-10 ms time error could also
be prevented there.

I have some comments, but, as is, feel free to add my ack.

Acked-by: Richard Cochran <richardcochran@...il.com>

> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 472591e..6e15fbb 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c

> @@ -359,6 +364,33 @@ u64 ntp_tick_length(void)
>  	return tick_length;
>  }
>  
> +/**
> + * get_leap_state - Returns the NTP leap state
> + * @next_leap_sec:	Next leapsecond in time64_t
> + * @next_leap_ktime:	Next leapsecond in ktime_t
> + *
> + * Provides NTP leapsecond state. Returns direction
> + * of the leapsecond adjustment as an integer.
> + */
> +int get_leap_state(time64_t *next_leap_sec, ktime_t *next_leap_ktime)
> +{
> +	int dir;
> +
> +	if ((time_state == TIME_INS) && (time_status & STA_INS)) {

This can be reduced to just one test on (time_state == TIME_INS).
If user spaces clears STA_INS, then you can immediately cancel the
leap second.

> +		dir = -1;
> +		*next_leap_sec = ntp_next_leap_sec;
> +		*next_leap_ktime = ktime_set(ntp_next_leap_sec, 0);
> +	} else if ((time_state == TIME_DEL) && (time_status & STA_DEL)) {
> +		dir = 1;
> +		*next_leap_sec = ntp_next_leap_sec;
> +		*next_leap_ktime = ktime_set(ntp_next_leap_sec, 0);
> +	} else {
> +		dir = 0;
> +		*next_leap_sec = TIME64_MAX;
> +		next_leap_ktime->tv64 = KTIME_MAX;
> +	}
> +	return dir;
> +}
>  
>  /*
>   * this routine handles the overflow of the microsecond field

> @@ -382,15 +414,21 @@ int second_overflow(unsigned long secs)
>  	 */
>  	switch (time_state) {
>  	case TIME_OK:
> -		if (time_status & STA_INS)
> +		if (time_status & STA_INS) {

The user sets STA_INS via adjtimex, but we don't change to TIME_INS
until the next tick.  Why not change immediately?  Then this funtion
would only need to check for TIME_INS && (secs % 86400 == 0)  and the
very unlikey TIME_DEL.

>  			time_state = TIME_INS;
> -		else if (time_status & STA_DEL)
> +			ntp_next_leap_sec = secs + SECS_PER_DAY -
> +						(secs % SECS_PER_DAY);
> +		} else if (time_status & STA_DEL) {
>  			time_state = TIME_DEL;
> +			ntp_next_leap_sec = secs + SECS_PER_DAY -
> +						 ((secs+1) % SECS_PER_DAY);
> +		}
>  		break;
>  	case TIME_INS:
> -		if (!(time_status & STA_INS))
> +		if (!(time_status & STA_INS)) {
> +			ntp_next_leap_sec = TIME64_MAX;
>  			time_state = TIME_OK;
> -		else if (secs % 86400 == 0) {
> +		} else if (secs % SECS_PER_DAY == 0) {
>  			leap = -1;
>  			time_state = TIME_OOP;
>  			printk_deferred(KERN_NOTICE

> @@ -711,6 +752,24 @@ int __do_adjtimex(struct timex *txc, struct timespec64 *ts, s32 *time_tai)
>  	if (!(time_status & STA_NANO))
>  		txc->time.tv_usec /= NSEC_PER_USEC;
>  
> +	/* Handle leapsec adjustments */

This block and its commnet rather confused me.  What this code
actually does is fix up the time value returned to the caller of
adjtimex, but only in the 1-10 millisecond window before the leap
second tick.

> +	if (unlikely(ts->tv_sec >= ntp_next_leap_sec)) {
> +		if ((time_state == TIME_INS) && (time_status & STA_INS)) {
> +			result = TIME_OOP;
> +			txc->tai++;
> +			txc->time.tv_sec--;
> +		}
> +		if ((time_state == TIME_DEL) && (time_status & STA_DEL)) {
> +			result = TIME_WAIT;
> +			txc->tai--;
> +			txc->time.tv_sec++;
> +		}
> +		if ((time_state == TIME_OOP) &&
> +					(ts->tv_sec == ntp_next_leap_sec)) {
> +			result = TIME_WAIT;
> +		}
> +	}
> +
>  	return result;
>  }

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/