linux-kernel - Re: [PATCH v2 2/4] KVM: introduce "xinterface" API for external interaction with guests

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:	Tue, 06 Oct 2009 10:22:05 -0400
From:	Gregory Haskins <gregory.haskins@...il.com>
To:	Avi Kivity <avi@...hat.com>
CC:	Gregory Haskins <ghaskins@...ell.com>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"alacrityvm-devel@...ts.sourceforge.net" 
	<alacrityvm-devel@...ts.sourceforge.net>,
	David Howells <dhowells@...hat.com>
Subject: Re: [PATCH v2 2/4] KVM: introduce "xinterface" API for external	interaction
 with guests

Gregory Haskins wrote:
> Avi Kivity wrote:
>> On 10/06/2009 01:57 AM, Gregory Haskins wrote:
>>> Avi Kivity wrote:
>>>   
>>>> On 10/02/2009 10:19 PM, Gregory Haskins wrote:

>>>>> +
>>>>> +static inline void
>>>>> +_kvm_xinterface_release(struct kref *kref)
>>>>> +{
>>>>> +    struct kvm_xinterface *intf;
>>>>> +    struct module *owner;
>>>>> +
>>>>> +    intf = container_of(kref, struct kvm_xinterface, kref);
>>>>> +
>>>>> +    owner = intf->owner;
>>>>> +    rmb();
>>>>>
>>>>>        
>>>> Why rmb?
>>>>      
>>> the intf->ops->release() line may invalidate the intf pointer, so we
>>> want to ensure that the read completes before the release() is called.
>>>
>>> TBH: I'm not 100% its needed, but I was being conservative.
>>>    
>> rmb()s are only needed if an external agent can issue writes, otherwise
>> you'd need one after every statement.
> 
> I was following lessons learned here:
> 
> http://lkml.org/lkml/2009/7/7/175
> 
> Perhaps mb() or barrier() are more appropriate than rmb()?  I'm CC'ing
> David Howells in case he has more insight.

BTW: In case it is not clear, the rationale as I understand it is we
worry about the case where one cpu reorders the read to be after the
->release(), and another cpu might grab the memory that was kfree()'d
within the ->release() and scribble something else on it before the read
completes.

I know rmb() typically needs to be paired with wmb() to be correct, so
you are probably right to say that the rmb() itself is not appropriate.
 This problem in general makes my head hurt, which is why I said I am
not 100% sure of what is required.  As David mentions, perhaps
"smp_mb()" is more appropriate for this application.  I also speculate
barrier() may be all that we need.

Kind Regards,
-Greg


Download attachment "signature.asc" of type "application/pgp-signature" (268 bytes)