linux-kernel - Re: [RFC Patch 1/10] Introducing generic hardware breakpoint handler interfaces

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: <20090201135433.GE7021@linux.vnet.ibm.com>
Date:	Sun, 1 Feb 2009 05:54:34 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	"K.Prasad" <prasad@...ux.vnet.ibm.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Roland McGrath <roland@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>, mingo@...e.hu,
	richardj_moore@...ibm.com, naren@...ux.vnet.ibm.com
Subject: Re: [RFC Patch 1/10] Introducing generic hardware breakpoint
	handler interfaces

On Fri, Jan 30, 2009 at 10:55:39AM -0500, Alan Stern wrote:
> On Fri, 30 Jan 2009, K.Prasad wrote:
> 
> > > A few RCU-related questions below.
> > > 
> > > 							Thanx, Paul
> 
> Paul, you've got to learn to trim your replies!  It's not nice to have
> to skim over hundreds and hundreds lines of quoted text while searching
> for your interpolated comments.  In fact, the phrase "needle in a 
> haystack" springs to mind...

I should have said "search for empty lines", but yes, I should have
trimmed a bit.  My apologies!!!

> > > > +	thr_kbpdata = chbi->cur_kbpdata;
> > > > +	barrier();
> > > 
> > > Couldn't the above two lines instead be:
> > > 
> > > 	thr_kbpdata = ACCESS_ONCE(chbi->cur_kbpdata);
> > > 
> > > This would prevent the pointer aliasing, but would make it very clear
> > > exactly how the compiler was to be restricted.
> > 
> > Ok. Using a barrier() could be an overkill. I will change it.
> 
> IIRC, the original code above was written before ACCESS_ONCE came into
> being.  But I could be wrong about that...

Could well be, ACCESS_ONCE() showed up in 2.6.24, and moved out of
rcupdate.h a couple of releases later.

> > > > +/*
> > > > + * Tell all CPUs to update their debug registers.
> > > > + *
> > > > + * The caller must hold hw_breakpoint_mutex.
> > > > + */
> > > > +static void update_all_cpus(void)
> > > > +{
> > > > +	/* We don't need to use any sort of memory barrier.  The IPI
> > > > +	 * carried out by on_each_cpu() includes its own barriers.
> > > > +	 */
> > > > +	on_each_cpu(update_this_cpu, NULL, 0);
> > > > +	synchronize_rcu();
> > > 
> > > Don't we need the rcu_read_lock() / rcu_read_unlock() pair from
> > > load_debug_registers() to move down into update_this_cpu() in order
> > > for this to be guaranteed to work?  As the code reads now, the
> > > update_this_cpu() calls running on other CPUs are not running under
> > > RCU protection, right?
> 
> Maybe I'm misunderstanding the question.  update_this_cpu() is called
> from only two places: on_each_cpu() as shown above, and
> load_debug_registers().  It seems clear that contexts resulting from
> on_each_cpu() don't need RCU protection, because on_each_cpu() won't
> return until those routines have completed.
> 
> This leaves only contexts resulting from load_debug_registers().  But
> the first thing load_debug_registers() does is disable local
> interrupts, thus blocking IPI delivery.  Hence any simultaneous
> on_each_cpu() won't complete until after load_debug_registers() is
> done.
> 
> So there doesn't seem to be any need for RCU protection in
> update_this_cpu().
> 
> > Yes, indeed. With the current implementation, there's a possibility of
> > two instances of update_this_cpu() function executing - one with an
> > rcu_read_lock() taken (when called from load_debug_registers) while the
> > other without (when invoked through update_all_cpus()).
> 
> No, this isn't possible unless I have misunderstood the nature of
> IPIs.  Isn't is true that calling local_irq_save() will block delivery
> of IPIs?

Touche!  ;-)

But in that case, why do you need the synchronize_rcu() following the
on_each_cpu() above?  Is this needed to make sure that any concurrent
load_debug_registers() call has completed?

							Thanx, Paul
--
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/