linux-kernel - Re: [Xen-devel] Re: [PATCH 09/14] xen/pvticketlock: Xen implementation for PV ticket locks

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:	Wed, 17 Nov 2010 10:34:39 +0000
From:	"Jan Beulich" <JBeulich@...ell.com>
To:	"Jeremy Fitzhardinge" <jeremy@...p.org>
Cc:	"Jeremy Fitzhardinge" <jeremy.fitzhardinge@...rix.com>,
	"Eric Dumazet" <dada1@...mosbay.com>, <xiyou.wangcong@...il.com>,
	"Peter Zijlstra" <peterz@...radead.org>,
	"Nick Piggin" <npiggin@...nel.dk>,
	"Srivatsa Vaddagiri" <vatsa@...ux.vnet.ibm.com>,
	"Linux Virtualization" <virtualization@...ts.linux-foundation.org>,
	"Xen-devel" <xen-devel@...ts.xensource.com>,
	"Mathieu Desnoyers" <mathieu.desnoyers@...ymtl.ca>,
	"Avi Kivity" <avi@...hat.com>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [Xen-devel] Re: [PATCH 09/14] xen/pvticketlock: Xen
	 implementation for PV ticket locks

>>> On 17.11.10 at 10:57, Jeremy Fitzhardinge <jeremy@...p.org> wrote:
> On 11/17/2010 12:52 AM, Jeremy Fitzhardinge wrote:
>> On 11/17/2010 12:11 AM, Jan Beulich wrote:
>>>>>> On 16.11.10 at 22:08, Jeremy Fitzhardinge <jeremy@...p.org> wrote:
>>>> +static void xen_lock_spinning(struct arch_spinlock *lock, unsigned want)
>>>>  {
>>>> -	struct xen_spinlock *xl = (struct xen_spinlock *)lock;
>>>> -	struct xen_spinlock *prev;
>>>>  	int irq = __get_cpu_var(lock_kicker_irq);
>>>> -	int ret;
>>>> +	struct xen_lock_waiting *w = &__get_cpu_var(lock_waiting);
>>>> +	int cpu = smp_processor_id();
>>>>  	u64 start;
>>>>  
>>>>  	/* If kicker interrupts not initialized yet, just spin */
>>>>  	if (irq == -1)
>>>> -		return 0;
>>>> +		return;
>>>>  
>>>>  	start = spin_time_start();
>>>>  
>>>> -	/* announce we're spinning */
>>>> -	prev = spinning_lock(xl);
>>>> +	w->want = want;
>>>> +	w->lock = lock;
>>>> +
>>>> +	/* This uses set_bit, which atomic and therefore a barrier */
>>>> +	cpumask_set_cpu(cpu, &waiting_cpus);
>>> Since you don't allow nesting, don't you need to disable
>>> interrupts before you touch per-CPU state?
>> Yes, I think you're right - interrupts need to be disabled for the bulk
>> of this function.
> 
> Actually, on second thoughts, maybe it doesn't matter so much.  The main
> issue is making sure that the interrupt will make the VCPU drop out of
> xen_poll_irq() - if it happens before xen_poll_irq(), it should leave
> the event pending, which will cause the poll to return immediately.  I
> hope.  Certainly disabling interrupts for some of the function will make
> it easier to analyze with respect to interrupt nesting.

That's not my main concern. Instead, what if you get interrupted
anywhere here, the interrupt handler tries to acquire another
spinlock and also has to go into the slow path? It'll overwrite part
or all of the outer context's state.

> Another issue may be making sure the writes and reads of "w->want" and
> "w->lock" are ordered properly to make sure that xen_unlock_kick() never
> sees an inconsistent view of the (lock,want) tuple.  The risk being that
> xen_unlock_kick() sees a random, spurious (lock,want) pairing and sends
> the kick event to the wrong VCPU, leaving the deserving one hung.

Yes, proper operation sequence (and barriers) is certainly
required here. If you allowed nesting, this may even become
simpler (as you'd have a single write making visible the new
"head" pointer, after having written all relevant fields of the
new "head" structure).

Jan

--
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/