linux-kernel - Re: [PATCH] tick: use READ_ONCE() to read jiffies in concurrent environment

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]

Date: Thu, 07 Mar 2024 22:15:20 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: linke li <lilinke99@...com>
Cc: lilinke99@...com, Frederic Weisbecker <frederic@...nel.org>, Ingo Molnar
 <mingo@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tick: use READ_ONCE() to read jiffies in concurrent
 environment

On Sun, Feb 25 2024 at 11:12, linke li wrote:
> In function tick_sched_do_timer(), jiffies is read using READ_ONCE()
> in line 224, while read directly in line 217
>
> 217	if (ts->last_tick_jiffies != jiffies) {
> 218		ts->stalled_jiffies = 0;
> 219		ts->last_tick_jiffies = READ_ONCE(jiffies);
> 220	} else {
> 221		if (++ts->stalled_jiffies == MAX_STALLED_JIFFIES) {
> 222			tick_do_update_jiffies64(now);
> 223			ts->stalled_jiffies = 0;
> 224			ts->last_tick_jiffies = READ_ONCE(jiffies);
> 225		}
> 226	}

Please do not paste the code which is changed by the patch into the
changelog. Describe the problem you are trying to solve.

> There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the
> KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")

The other patch has absolutely nothing to do with this code and . Describe
the problem and the solution.

> This patch find two read of same variable while one is protected, another
> is not. And READ_ONCE() is added to protect.

This patch finds nothing. Explain it correctly why it matters that the
first read is not marked READ_ONCE(). Is this solving a correctness
problem or are you adding it just to shut up the KCSAN warning?

> @@ -214,7 +214,7 @@ static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
>  	 * If the jiffies update stalled for too long (timekeeper in stop_machine()
>  	 * or VMEXIT'ed for several msecs), force an update.
>  	 */
> -	if (ts->last_tick_jiffies != jiffies) {
> +	if (ts->last_tick_jiffies != READ_ONCE(jiffies)) {
>  		ts->stalled_jiffies = 0;
>  		ts->last_tick_jiffies = READ_ONCE(jiffies);
>  	} else {

Thanks,

        tglx