linux-kernel - Re: [PATCH LINUX v5] xen: event channel arrays are xen_ulong

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: <20130305140452.GE2589@phenom.dumpdata.com>
Date:	Tue, 5 Mar 2013 09:04:52 -0500
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	Ian Campbell <ian.campbell@...rix.com>
Cc:	Rob Herring <robherring2@...il.com>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"Keir (Xen.org)" <keir@....org>,
	Stefano Stabellini <Stefano.Stabellini@...citrix.com>,
	"Tim (Xen.org)" <tim@....org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Jan Beulich <JBeulich@...e.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Nicolas Pitre <nico@...xnic.net>,
	Will Deacon <will.deacon@....com>
Subject: Re: [PATCH LINUX v5] xen: event channel arrays are xen_ulong_t and
 not unsigned long

On Tue, Mar 05, 2013 at 03:56:41AM +0000, Ian Campbell wrote:
> > > diff --git a/arch/arm/include/asm/xen/events.h b/arch/arm/include/asm/xen/events.h
> > > index 94b4e90..5c27696 100644
> > > --- a/arch/arm/include/asm/xen/events.h
> > > +++ b/arch/arm/include/asm/xen/events.h
> > > @@ -15,4 +15,26 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
> > >  	return raw_irqs_disabled_flags(regs->ARM_cpsr);
> > >  }
> > >  
> > > +/*
> > > + * We cannot use xchg because it does not support 8-byte
> > > + * values. However it is safe to use {ldr,dtd}exd directly because all
> > > + * platforms which Xen can run on support those instructions.
> > 
> > Why does atomic64_cmpxchg not work here?
> 
> Just that we don't want/need the cmp aspect, we don't mind if an extra
> bit gets set as we read the value, so long as we atomically read and set
> to zero.
> 
> > > + */
> > > +static inline xen_ulong_t xchg_xen_ulong(xen_ulong_t *ptr, xen_ulong_t val)
> > > +{
> > > +	xen_ulong_t oldval;
> > > +	unsigned int tmp;
> > > +
> > > +	wmb();
> > 
> > Based on atomic64_cmpxchg implementation, you could use smp_mb here
> > which avoids an outer cache flush.
> 
> Good point.
> 
> > > +	asm volatile("@ xchg_xen_ulong\n"
> > > +		"1:     ldrexd  %0, %H0, [%3]\n"
> > > +		"       strexd  %1, %2, %H2, [%3]\n"
> > > +		"       teq     %1, #0\n"
> > > +		"       bne     1b"
> > > +		: "=&r" (oldval), "=&r" (tmp)
> > > +		: "r" (val), "r" (ptr)
> > > +		: "memory", "cc");
> > 
> > And a smp_mb is needed here.
> 
> I think for the specific caller which we have here it isn't strictly
> necessary, but for generic correctness I think you are right.
> 
> Thanks for reviewing.
> 
> Konrad, IIRC you have already picked this up (and sent to Linus?) so an

Yes.
> incremental fix is required? See below.

Why don't I wait a bit and wait until you are back from conferences and
can post a nice series that fixes the smp_wmb() and also the atomic one
and has been run-time tested with Xen on ARM.
--
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/