linux-kernel - Re: [PATCH RFC kenrel/rcu] Eliminate BUG

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:   Mon, 22 Oct 2018 08:56:44 -0700
From:   "Paul E. McKenney" <paulmck@...ux.ibm.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     peterz@...radead.org, linux-kernel@...r.kernel.org,
        josh@...htriplett.org, rostedt@...dmis.org,
        mathieu.desnoyers@...icios.com, jiangshanlai@...il.com
Subject: Re: [PATCH RFC kenrel/rcu] Eliminate BUG_ON() for sync.c

On Mon, Oct 22, 2018 at 05:24:07PM +0200, Oleg Nesterov wrote:
> On 10/22, Paul E. McKenney wrote:
> >
> > The sync.c file has a number of calls to BUG_ON(), which panics the
> > kernel, which is not a good
> 
> Agreed.
> 
> I added these BUG_ON's for documentation when I was prototyping this code,
> perhaps we can simply remove them.

I do like this approach.  ;-)

> > @@ -125,12 +125,12 @@ void rcu_sync_enter(struct rcu_sync *rsp)
> >  		rsp->gp_state = GP_PENDING;
> >  	spin_unlock_irq(&rsp->rss_lock);
> >  
> > -	BUG_ON(need_wait && need_sync);
> > -
> >  	if (need_sync) {
> >  		gp_ops[rsp->gp_type].sync();
> >  		rsp->gp_state = GP_PASSED;
> >  		wake_up_all(&rsp->gp_wait);
> > +		if (WARN_ON_ONCE(need_wait))
> > +			wait_event(rsp->gp_wait, rsp->gp_state == GP_PASSED);
> 
> This wait_event(gp_state == GP_PASSED) is pointless, note that this branch
> does gp_state = GP_PASSED 2 lines above.

OK, I have removed this one.

> And if we add WARN_ON_ONCE(need_wait), then we should probably also add
> WARN_ON_ONCE(need_sync) into the next "if (need_wait)" branch just for
> symmetry.

But in that case, the earlier "if" prevents "need_sync" from ever getting
there, unless I lost the thread here.

> So I'd suggest to either turn that BUG_ON(need_wait && need_sync) above
> into WARN_ON_ONCE(wait && sync) or simply remove it.

I chose WARN_ON_ONCE() for this one.

> Again, the only purpose of this BUG_ON() is to explain to the reader that
> it is not (must not be) possible that, say, gp_state == GP_IDLE while
> gp_count != 0.

Good point!

Should I remove the others?

> ----------------------------------------------------------------------------
> Damn.
> 
> This suddenly reminds me that I rewrote this code completely, and you even
> reviewed the new implementation and (iirc) acked it!
> 
> However, I failed to force myself to rewrite the comments, and that is why
> I didn't send the "official" patch :/
> 
> May be some time...

Could you please point me at the last email thread?  Yes, I should be
able to find it, but I would probably get the wrong one.  :-/

							Thanx, Paul