Re: Fwd: New Defects reported by Coverity Scan for XenProject

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2024 13:19, Andrew Cooper wrote:
> Take 2, hopefully with Stewart's correct email address this time.
> 
> ~Andrew
> 
> On 28/02/2024 12:17 pm, Andrew Cooper wrote:
>> Not sure how well this is going to be formatted, but there's one new and
>> potentially interesting issue found by Coverity.

To be honest I didn't consider this interesting at all, but instead a false
positive due to limited insight that the tool has. But maybe I was wrong
and you see something I didn't see? vpci_process_pending() is vCPU-local
(run from the guest resume path), and hence there simply are no two threads
who want to look at the field. Storing NULL into it is merely a kind of
progress indicator, relevant to the given vCPU only.

Jan

>> ----8<----
>>
>> New defect(s) Reported-by: Coverity Scan
>> Showing 1 of 1 defect(s)
>>
>>
>> ** CID 1592633: (LOCK_EVASION)
>> /xen/drivers/vpci/header.c: 229 in vpci_process_pending()
>> /xen/drivers/vpci/header.c: 189 in vpci_process_pending()
>> /xen/drivers/vpci/header.c: 239 in vpci_process_pending()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1592633: (LOCK_EVASION)
>> /xen/drivers/vpci/header.c: 229 in vpci_process_pending()
>> 223 224 /* Clean all the rangesets */
>> 225 for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> 226 if ( !rangeset_is_empty(header->bars[i].mem) )
>> 227 rangeset_purge(header->bars[i].mem);
>> 228
>>>>> CID 1592633: (LOCK_EVASION)
>>>>> Thread1 sets "pdev" to a new value. Now the two threads have an
>>>>> inconsistent view of "pdev" and updates to fields of "pdev" or
>>>>> fields correlated with "pdev" may be lost.
>> 229 v->vpci.pdev = NULL;
>> 230 231 read_unlock(&v->domain->pci_lock);
>> 232 233 if ( !is_hardware_domain(v->domain) )
>> 234 domain_crash(v->domain);
>> /xen/drivers/vpci/header.c: 189 in vpci_process_pending()
>> 183 return false;
>> 184 185 read_lock(&v->domain->pci_lock);
>> 186 187 if ( !pdev->vpci || (v->domain != pdev->domain) )
>> 188 {
>>>>> CID 1592633: (LOCK_EVASION)
>>>>> Thread1 sets "pdev" to a new value. Now the two threads have an
>>>>> inconsistent view of "pdev" and updates to fields of "pdev" or
>>>>> fields correlated with "pdev" may be lost.
>> 189 v->vpci.pdev = NULL;
>> 190 read_unlock(&v->domain->pci_lock);
>> 191 return false;
>> 192 }
>> 193 194 header = &pdev->vpci->header;
>> /xen/drivers/vpci/header.c: 239 in vpci_process_pending()
>> 233 if ( !is_hardware_domain(v->domain) )
>> 234 domain_crash(v->domain);
>> 235 236 return false;
>> 237 }
>> 238 }
>>>>> CID 1592633: (LOCK_EVASION)
>>>>> Thread1 sets "pdev" to a new value. Now the two threads have an
>>>>> inconsistent view of "pdev" and updates to fields of "pdev" or
>>>>> fields correlated with "pdev" may be lost.
>> 239 v->vpci.pdev = NULL;
>> 240 241 spin_lock(&pdev->vpci->lock);
>> 242 modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
>> 243 spin_unlock(&pdev->vpci->lock);
>> 244
>>
>