netdev - Re: [PATCH] netpoll: use non-BH variant of 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]

Date:	Fri, 13 Aug 2010 09:29:12 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	David Miller <davem@...emloft.net>, linville@...driver.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH] netpoll: use non-BH variant of RCU

On Fri, Aug 13, 2010 at 10:39:04AM -0400, Herbert Xu wrote:
> On Thu, Aug 12, 2010 at 08:42:13AM -0700, Paul E. McKenney wrote:
> >
> > +/**
> > + * rcu_read_lock_bh_irqsoff() - mark the beginning of an RCU-bh critical section
> > + *
> > + * This is equivalent of rcu_read_lock_bh(), but to be used where the
> > + * caller either is in an irq handler or has irqs disabled.  Note that
> > + * this function assumes that PREEMPT_RT kernels run irq handlers at
> > + * higher priority than softirq handlers!
> > + */
> > +static inline void rcu_read_lock_bh_irqsoff(void)
> > +{
> > +	rcu_read_unlock_bh_irqsoff_check();
> > +	__acquire(RCU_BH);
> > +	rcu_read_acquire_bh();
> > +}
> 
> Thanks for the patch Paul!
> 
> But this doesn't really solve the problem for netif_rx.  The reason
> is that netif_rx can either be called with IRQs on OR off.  So we
> need to take the right precautions in the case where IRQs are
> enabled along with BH.

Interesting...

Is it possible that IRQs are off at rcu_read_lock_bh_irqsoff() time, but
enabled by the time we get to rcu_read_unlock_bh_irqsoff()?  I hope not,
but have to ask.  If I am guaranteed of the same state in both cases,
I can do something like the following:

static inline void rcu_read_lock_bh_irqsoff(void)
{
	if (!in_irq() && !irqs_disabled())
		local_bh_disable();
	__acquire(RCU_BH);
	rcu_read_acquire_bh();
}

static inline void rcu_read_unlock_bh_irqsoff(void)
{
	rcu_read_release_bh();
	__release(RCU_BH);
	if (!in_irq() && !irqs_disabled())
		local_bh_enable();
}

If the state can change in the RCU-bh read-side critical section, then
I would have to record the state in the task structure or some such.

But all in all, mightn't it be easier to remove the checks from
_local_bh_enable(), and then just use rcu_read_lock_bh()?  Have those
checks really been that helpful in finding bugs?  ;-)

							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html