linux-kernel - Re: [PATCH RFC v2 rcu] Fix get_state_synchronize_rcu

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: <Z5T7kF7LKIuk05FR@pavilion.home>
Date: Sat, 25 Jan 2025 15:56:16 +0100
From: Frederic Weisbecker <frederic@...nel.org>
To: "Paul E. McKenney" <paulmck@...nel.org>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org, kernel-team@...a.com,
	rostedt@...dmis.org
Subject: Re: [PATCH RFC v2 rcu] Fix get_state_synchronize_rcu_full() GP-start
 detection

Le Fri, Jan 24, 2025 at 04:01:55PM -0800, Paul E. McKenney a écrit :
> On Sat, Jan 25, 2025 at 12:03:58AM +0100, Frederic Weisbecker wrote:
> > Le Fri, Dec 13, 2024 at 11:49:49AM -0800, Paul E. McKenney a écrit :
> > > diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> > > index 2f9c9272cd486..d2a91f705a4ab 100644
> > > --- a/kernel/rcu/rcu.h
> > > +++ b/kernel/rcu/rcu.h
> > > @@ -162,7 +162,7 @@ static inline bool rcu_seq_done_exact(unsigned long *sp, unsigned long s)
> > >  {
> > >  	unsigned long cur_s = READ_ONCE(*sp);
> > >  
> > > -	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (2 * RCU_SEQ_STATE_MASK + 1));
> > > +	return ULONG_CMP_GE(cur_s, s) || ULONG_CMP_LT(cur_s, s - (3 * RCU_SEQ_STATE_MASK + 1));
> > 
> > This might need a comment.
> 
> Good point!  Would you like to propose one?

Ok.

> 
> > The way I understand it is that rcu_state.gp_seq might be seen started while
> > root_rnp->gp_seq is not. So rcu_seq_snap() on the started rcu_state.gp_seq
> > may return maximum 2 full GPs ahead of root_rnp->gp_seq. And therefore it takes below
> > 2 GPs to safely deduce we wrapped around.
> 
> Exactly!
> 
> > Should it be ULONG_CMP_LT(cur_s, s - (2 * (RCU_SEQ_STATE_MASK + 1))) ?
> 
> Quite possibly.  I freely admit that I allowed a bit of slop because
> time was of the essence (holidays and all that) and also it does not
> hurt much to lose a couple of counts out of a 2^32 cycle, to say nothing
> of the common-case 2^64 cycle.  It would not hurt to be exact, but it
> would be necessary to convince ourselves that we were not off by one in
> the wrong direction.
> 
> I would be happy to see a patch, as long as it was sufficiently
> convincing.

I'm not so much concerned about being exact but rather about making
sure we still understand what we did within one year. We can leave one
more grace period than what we expect out of paranoia but, the most
important is that we comment about what we expect and why. Let me
prepare a patch for that.

In the meantime for your patch:

   Reviewed-by: Frederic Weisbecker <frederic@...nel.org>


> 
> > Or am I missing something?
> 
> Not that I can see.  So the answer is probably "yes".  ;-)

Thanks! :-)

> 
> 							Thanx, Paul