From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject) Date: Wed, 3 Feb 2016 12:24:48 +0000 Message-ID: <56B1F190.8070600@citrix.com> References: <56b180c017d5f_214fb5b3143623f@ss1435.mail> <1454496349.25207.54.camel@citrix.com> <56B1DB64.8040402@citrix.com> <1454497213.25207.62.camel@citrix.com> <56B1F0CC.4090708@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56B1F0CC.4090708@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell , Malcolm Crossley Cc: George Dunlap , Jan Beulich , xen-devel List-Id: xen-devel@lists.xenproject.org On 03/02/16 12:21, Andrew Cooper wrote: > On 03/02/16 11:00, Ian Campbell wrote: >> On Wed, 2016-02-03 at 10:50 +0000, Andrew Cooper wrote: >>> On 03/02/16 10:45, Ian Campbell wrote: >>>> On Tue, 2016-02-02 at 20:23 -0800, scan-admin@coverity.com wrote: >>>>> * CID 1351223: Concurrent data access violations (MISSING_LOCK) >>>>> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock() >>>> Coverity seems to think this is new in 41b0aa569adb..9937763265d, >>>> presumably due to >>>> >>>> commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee >>>> Author: Malcolm Crossley >>>> Date: Fri Jan 22 16:04:41 2016 +0100 >>>> >>>> rwlock: add per-cpu reader-writer lock infrastructure >>> Expected behaviour. writer_activating is expected to only be written >>> under lock, but read without lock. >> I suppose this is something we should eventually model? > Short of annotating the source code with Coverity comments (which has > already been objected to), I don't see a way. > > This issue is Coverity (correctly) observing the behaviour of the > function, and coming to the wrong conclusion. The modelling file is > used to correct the interpretation of the behaviour of the function. > >> Would you typically mark this as "False positive" or "Intentional"? > I would err on the side of Intentional. > > The analysis of the issue was correct; that data was accessed both with > and without the lock, and that this usually means a data race condition. > > In this case, it is a deliberate algorithm decision to have the data > access like this. > >> I just marked a couple of libxl ones about taking ctx->lock (which is >> recursive) twice as "False positive", but perhaps "Intentional" is the >> correct designation there? > There is an attempt to model this in the model file, but it clearly > isn't taking. (I meant to say as well) This I would err in the side of false positive, with "modelling required" as a reason. The lock is a recursive lock and Coverity should be able to spot this fact, but can't for some reason. ~Andrew