linux-kernel - Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server

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, 03 Nov 2009 14:50:37 -0500
From:	Gregory Haskins <gregory.haskins@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	"Michael S. Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org,
	virtualization@...ts.linux-foundation.org, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, mingo@...e.hu, linux-mm@...ck.org,
	akpm@...ux-foundation.org, hpa@...or.com,
	Rusty Russell <rusty@...tcorp.com.au>, s.hetze@...ux-ag.com,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server

Eric Dumazet wrote:
> Gregory Haskins a écrit :
>> Gregory Haskins wrote:
>>> Eric Dumazet wrote:
>>>> Michael S. Tsirkin a écrit :

>>>> using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect
>>>> that your use of RCU is not correct.
>>>>
>>>> 1) rcu_dereference() should be done inside a read_rcu_lock() section, and
>>>>    we are not allowed to sleep in such a section.
>>>>    (Quoting Documentation/RCU/whatisRCU.txt :
>>>>      It is illegal to block while in an RCU read-side critical section, )
>>>>
>>>> 2) mutex_lock() can sleep (ie block)
>>>>
>>> Michael,
>>>   I warned you that this needed better documentation ;)
>>>
>>> Eric,
>>>   I think I flagged this once before, but Michael convinced me that it
>>> was indeed "ok", if but perhaps a bit unconventional.  I will try to
>>> find the thread.
>>>
>>> Kind Regards,
>>> -Greg
>>>
>> Here it is:
>>
>> http://lkml.org/lkml/2009/8/12/173
>>
> 
> Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use.
> People wanting to use RCU do a grep on kernel sources to find how to correctly
> use RCU.
> 
> Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Yes, I would tend to agree with you.  In fact, I think I suggested that
a normal barrier should be used instead of abusing rcu_dereference().

But as far as his code is concerned, I think it technically works
properly, and that was my main point.  Also note that the usage
rcu_dereference+mutex_lock() are not necessarily broken, per se:  it
could be an srcu-based critical section created by the caller, for
instance.  It would be perfectly legal to sleep on the mutex if that
were the case.

To me, the bigger issue is that the rcu_dereference() without any
apparent hint of a corresponding RSCS is simply confusing as a reviewer.
 smp_rmb() (or whatever is proper in this case) is probably more
appropriate.

Kind Regards,
-Greg



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