Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

George,

Looks like xentrace is the only maintained component which uses this. so
tag ;-)

On Tue, 2016-02-02 at 20:23 -0800, scan-admin@coverity.com wrote:
> * CID 1351228:    (RESOURCE_LEAK)
> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()

Coverity is reporting these as new in 41b0aa569adb..9937763265d9 although
the file hasn't changed. However it does look correct that t_info is being
leaked by various paths in this function.

> 
> 
> _________________________________________________________________________
> _______________________________
> *** CID 1351228:    (RESOURCE_LEAK)
> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
> 67     
> 68         t_info = xc_map_foreign_range(xch, DOMID_XEN,
> 69                         sysctl.u.tbuf_op.size, PROT_READ | PROT_WRITE,
> 70                         sysctl.u.tbuf_op.buffer_mfn);
> 71     
> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
> >>>     CID 1351228:    (RESOURCE_LEAK)
> >>>     Variable "t_info" going out of scope leaks the storage it points
> to.
> 73             return -1;
> 74     
> 75         *size = t_info->tbuf_size;
> 76     
> 77         return 0;
> 78     }
> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
> 71     
> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
> 73             return -1;
> 74     
> 75         *size = t_info->tbuf_size;
> 76     
> >>>     CID 1351228:    (RESOURCE_LEAK)
> >>>     Variable "t_info" going out of scope leaks the storage it points
> to.
> 77         return 0;
> 78     }
> 79     
> 80     int xc_tbuf_enable(xc_interface *xch, unsigned long pages,
> unsigned long *mfn,
> 81                        unsigned long *size)
> 82     {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

On 03/02/16 10:37, Ian Campbell wrote:
> George,
>
> Looks like xentrace is the only maintained component which uses this. so
> tag ;-)
>
> On Tue, 2016-02-02 at 20:23 -0800, scan-admin@coverity.com wrote:
>> * CID 1351228:    (RESOURCE_LEAK)
>> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
>> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
> Coverity is reporting these as new in 41b0aa569adb..9937763265d9 although
> the file hasn't changed. However it does look correct that t_info is being
> leaked by various paths in this function.

It is "new"ly spotted because xc_map_foreign_range() is now a straight
function call rather than a function pointer pointing at functions with
different behaviours.

~Andrew

Re: Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 10:42 +0000, Andrew Cooper wrote:
> On 03/02/16 10:37, Ian Campbell wrote:
> > George,
> > 
> > Looks like xentrace is the only maintained component which uses this.
> > so
> > tag ;-)
> > 
> > On Tue, 2016-02-02 at 20:23 -0800, scan-admin@coverity.com wrote:
> > > * CID 1351228:    (RESOURCE_LEAK)
> > > /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
> > > /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
> > Coverity is reporting these as new in 41b0aa569adb..9937763265d9
> > although
> > the file hasn't changed. However it does look correct that t_info is
> > being
> > leaked by various paths in this function.
> 
> It is "new"ly spotted because xc_map_foreign_range() is now a straight
> function call rather than a function pointer pointing at functions with
> different behaviours.

That makes sense, thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[George Dunlap]

On Wed, Feb 3, 2016 at 10:37 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> George,
>
> Looks like xentrace is the only maintained component which uses this. so
> tag ;-)

OK -- I think this will only be called once, so it's only leaking a
page or two of virtual address space until the process dies (which in
the case of an error will be immediate).  But certainly worth fixing.
:-)

I'll put this on a list of clean-ups; maybe we can give it to the next
OPW / GSoC candidate who shows up (if I don't get to it earlier).

 -George

>
> On Tue, 2016-02-02 at 20:23 -0800, scan-admin@coverity.com wrote:
>> * CID 1351228:    (RESOURCE_LEAK)
>> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
>> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
>
> Coverity is reporting these as new in 41b0aa569adb..9937763265d9 although
> the file hasn't changed. However it does look correct that t_info is being
> leaked by various paths in this function.
>
>>
>>
>> _________________________________________________________________________
>> _______________________________
>> *** CID 1351228:    (RESOURCE_LEAK)
>> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
>> 67
>> 68         t_info = xc_map_foreign_range(xch, DOMID_XEN,
>> 69                         sysctl.u.tbuf_op.size, PROT_READ | PROT_WRITE,
>> 70                         sysctl.u.tbuf_op.buffer_mfn);
>> 71
>> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
>> >>>     CID 1351228:    (RESOURCE_LEAK)
>> >>>     Variable "t_info" going out of scope leaks the storage it points
>> to.
>> 73             return -1;
>> 74
>> 75         *size = t_info->tbuf_size;
>> 76
>> 77         return 0;
>> 78     }
>> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
>> 71
>> 72         if ( t_info == NULL || t_info->tbuf_size == 0 )
>> 73             return -1;
>> 74
>> 75         *size = t_info->tbuf_size;
>> 76
>> >>>     CID 1351228:    (RESOURCE_LEAK)
>> >>>     Variable "t_info" going out of scope leaks the storage it points
>> to.
>> 77         return 0;
>> 78     }
>> 79
>> 80     int xc_tbuf_enable(xc_interface *xch, unsigned long pages,
>> unsigned long *mfn,
>> 81                        unsigned long *size)
>> 82     {
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Leak in xc_dom_load_hvm_kernel() (Was; Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

Roger,

On Tue, 2016-02-02 at 20:23 -0800, scan-admin@coverity.com wrote:
> ** CID 1351227:    (RESOURCE_LEAK)
> /tools/libxc/xc_dom_hvmloader.c: 260 in xc_dom_load_hvm_kernel()
> /tools/libxc/xc_dom_hvmloader.c: 270 in xc_dom_load_hvm_kernel()
> /tools/libxc/xc_dom_hvmloader.c: 277 in xc_dom_load_hvm_kernel()

Looks like this came from ad787bafcd2a3058f0f37f2fe84931bd5136bde9?

> ________________________________________________________________________________________________________
> *** CID 1351227:    (RESOURCE_LEAK)
> /tools/libxc/xc_dom_hvmloader.c: 260 in xc_dom_load_hvm_kernel()
> 254         elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom);
> 255     
> 256         rc = elf_load_binary(elf);
> 257         if ( rc < 0 )
> 258         {
> 259             DOMPRINTF("%s: failed to load elf binary", __func__);
> >>>     CID 1351227:    (RESOURCE_LEAK)
> >>>     Variable "entries" going out of scope leaks the storage it points to.
> 260             return rc;
> 261         }
> 262     
> 263         munmap(elf->dest_base, elf->dest_size);
> 264     
> 265         rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, &m_start,
> /tools/libxc/xc_dom_hvmloader.c: 270 in xc_dom_load_hvm_kernel()
> 264     
> 265         rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, &m_start,
> 266                           &m_end);
> 267         if ( rc != 0 )
> 268         {
> 269             DOMPRINTF("%s: insufficient space to load modules.", __func__);
> >>>     CID 1351227:    (RESOURCE_LEAK)
> >>>     Variable "entries" going out of scope leaks the storage it points to.
> 270             return rc;
> 271         }
> 272     
> 273         rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
> 274         if ( rc != 0 )
> 275         {
> /tools/libxc/xc_dom_hvmloader.c: 277 in xc_dom_load_hvm_kernel()
> 271         }
> 272     
> 273         rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
> 274         if ( rc != 0 )
> 275         {
> 276             DOMPRINTF("%s: unable to load modules.", __func__);
> >>>     CID 1351227:    (RESOURCE_LEAK)
> >>>     Variable "entries" going out of scope leaks the storage it points to.
> 277             return rc;
> 278         }
> 279     
> 280         dom->parms.phys_entry = elf_uval(elf, elf->ehdr, e_entry);
> 281     
> 282         free(entries);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH] libxc: fix leak in xc_dom_load_hvm_kernel error path

[Roger Pau Monne]

Error path in xc_dom_load_hvm_kernel needs to use the 'error' label instead
of directly returning. This is needed so the entries local variable is
freed.

Coverity-ID: 1351227
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_hvmloader.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 79a3b99..330d5e8 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -257,7 +257,7 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
     if ( rc < 0 )
     {
         DOMPRINTF("%s: failed to load elf binary", __func__);
-        return rc;
+        goto error;
     }
 
     munmap(elf->dest_base, elf->dest_size);
@@ -267,14 +267,14 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct xc_dom_image *dom)
     if ( rc != 0 )
     {
         DOMPRINTF("%s: insufficient space to load modules.", __func__);
-        return rc;
+        goto error;
     }
 
     rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
     if ( rc != 0 )
     {
         DOMPRINTF("%s: unable to load modules.", __func__);
-        return rc;
+        goto error;
     }
 
     dom->parms.phys_entry = elf_uval(elf, elf->ehdr, e_entry);
-- 
2.5.4 (Apple Git-61)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxc: fix leak in xc_dom_load_hvm_kernel error path

[Ian Campbell]

On Wed, 2016-02-03 at 11:59 +0100, Roger Pau Monne wrote:
> Error path in xc_dom_load_hvm_kernel needs to use the 'error' label
> instead
> of directly returning. This is needed so the entries local variable is
> freed.
> 
> Coverity-ID: 1351227
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked + applied, thanks.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

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 <malcolm.crossley@citrix.com>
Date:   Fri Jan 22 16:04:41 2016 +0100

    rwlock: add per-cpu reader-writer lock infrastructure

> _________________________________________________________________________
> _______________________________
> *** CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> 356                     percpu_rwlock_t *percpu_rwlock)
> 357     {
> 358         /* Validate the correct per_cpudata variable has been
> provided. */
> 359         _percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
> 360     
> 361         ASSERT(percpu_rwlock->writer_activating);
> >>>     CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> >>>     Accessing "percpu_rwlock->writer_activating" without holding lock
> "percpu_rwlock.rwlock". Elsewhere, "percpu_rwlock.writer_activating" is
> accessed with "percpu_rwlock.rwlock" held 1 out of 2 times (1 of these
> accesses strongly imply that it is necessary).
> 362         percpu_rwlock->writer_activating = 0;
> 363         write_unlock(&percpu_rwlock->rwlock);
> 364     }
> 365     
> 366     #define percpu_rw_is_write_locked(l)        
> _rw_is_write_locked(&((l)->rwlock))
> 367     

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 10:45 +0000, 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 <malcolm.crossley@citrix.com>
> Date:   Fri Jan 22 16:04:41 2016 +0100
> 
>     rwlock: add per-cpu reader-writer lock infrastructure

It also reports this one, but I suppose this is a false +ve given the name
of the function.

(Also note "simulatenously" should be "simultaneously")

    ** CID 1351220:  Program hangs  (LOCK)
    /xen/include/xen/spinlock.h: 310 in _percpu_read_lock()


    ________________________________________________________________________________________________________
    *** CID 1351220:  Program hangs  (LOCK)
    /xen/include/xen/spinlock.h: 310 in _percpu_read_lock()
    304          * Detect using a second percpu_rwlock_t simulatenously and fallback
    305          * to standard read_lock.
    306          */
    307         if ( unlikely(this_cpu_ptr(per_cpudata) != NULL ) )
    308         {
    309             read_lock(&percpu_rwlock->rwlock);
    >>>     CID 1351220:  Program hangs  (LOCK)
    >>>     Returning without unlocking "percpu_rwlock->rwlock".
    310             return;
    311         }
    312     
    313         /* Indicate this cpu is reading. */
    314         this_cpu_ptr(per_cpudata) = percpu_rwlock;
    315         smp_mb();


> 
> > _______________________________________________________________________
> > __
> > _______________________________
> > *** CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> > /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> > 356                     percpu_rwlock_t *percpu_rwlock)
> > 357     {
> > 358         /* Validate the correct per_cpudata variable has been
> > provided. */
> > 359         _percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
> > 360     
> > 361         ASSERT(percpu_rwlock->writer_activating);
> > > > >      CID 1351223:  Concurrent data access violations 
> > > > > (MISSING_LOCK)
> > > > >      Accessing "percpu_rwlock->writer_activating" without holding
> > > > > lock
> > "percpu_rwlock.rwlock". Elsewhere, "percpu_rwlock.writer_activating" is
> > accessed with "percpu_rwlock.rwlock" held 1 out of 2 times (1 of these
> > accesses strongly imply that it is necessary).
> > 362         percpu_rwlock->writer_activating = 0;
> > 363         write_unlock(&percpu_rwlock->rwlock);
> > 364     }
> > 365     
> > 366     #define percpu_rw_is_write_locked(l)        
> > _rw_is_write_locked(&((l)->rwlock))

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

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 <malcolm.crossley@citrix.com>
> 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.

~Andrew

>
>> _________________________________________________________________________
>> _______________________________
>> *** CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
>> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
>> 356                     percpu_rwlock_t *percpu_rwlock)
>> 357     {
>> 358         /* Validate the correct per_cpudata variable has been
>> provided. */
>> 359         _percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
>> 360     
>> 361         ASSERT(percpu_rwlock->writer_activating);
>>>>>      CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
>>>>>      Accessing "percpu_rwlock->writer_activating" without holding lock
>> "percpu_rwlock.rwlock". Elsewhere, "percpu_rwlock.writer_activating" is
>> accessed with "percpu_rwlock.rwlock" held 1 out of 2 times (1 of these
>> accesses strongly imply that it is necessary).
>> 362         percpu_rwlock->writer_activating = 0;
>> 363         write_unlock(&percpu_rwlock->rwlock);
>> 364     }
>> 365     
>> 366     #define percpu_rw_is_write_locked(l)        
>> _rw_is_write_locked(&((l)->rwlock))
>> 367     
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

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 <malcolm.crossley@citrix.com>
> > 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?

Would you typically mark this as "False positive" or "Intentional"?

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?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

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 <malcolm.crossley@citrix.com>
>>> 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.

~Andrew

Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

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 <malcolm.crossley@citrix.com>
>>>> 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

Re: missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 12:24 +0000, Andrew Cooper wrote:
> 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 <malcolm.crossley@citrix.com>
> > > > > 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.

Done. I linked to your explanation in the comments.

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

Good idea, I'll update those.

Ian

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel