Hi Cédric,
On 12/21/23 22:23, Cédric Le Goater wrote:
> On 12/21/23 18:14, Eric Auger wrote:
>> Hi Cédric,
>>
>> On 12/21/23 17:00, Cédric Le Goater wrote:
>>> [ ... ]
>>>
>>>
>>>> +static void iommufd_backend_init(Object *obj)
>>>> +{
>>>> + IOMMUFDBackend *be = IOMMUFD_BACKEND(obj);
>>>> +
>>>> + be->fd = -1;
>>>> + be->users = 0;
>>>> + be->owned = true;
>>>> + qemu_mutex_init(&be->lock);> +}
>>>> +
>>>> +static void iommufd_backend_finalize(Object *obj)
>>>> +{
>>>> + IOMMUFDBackend *be = IOMMUFD_BACKEND(obj);
>>>> +
>>>> + if (be->owned) {
>>>> + close(be->fd);
>>>> + be->fd = -1;
>>>> + }
>>>> +}
>>>> +
>>>> +static void iommufd_backend_set_fd(Object *obj, const char *str,
>>>> Error **errp)
>>>> +{
>>>> + IOMMUFDBackend *be = IOMMUFD_BACKEND(obj);
>>>> + int fd = -1;
>>>> +
>>>> + fd = monitor_fd_param(monitor_cur(), str, errp);
>>>> + if (fd == -1) {
>>>> + error_prepend(errp, "Could not parse remote object fd %s:",
>>>> str);
>>>> + return;
>>>> + }
>>>> + qemu_mutex_lock(&be->lock);
>>>> + be->fd = fd;
>>>> + be->owned = false;
>>>> + qemu_mutex_unlock(&be->lock);
>>>> + trace_iommu_backend_set_fd(be->fd);
>>>> +}
>>>> +
>>>> +static bool iommufd_backend_can_be_deleted(UserCreatable *uc)
>>>> +{
>>>> + IOMMUFDBackend *be = IOMMUFD_BACKEND(uc);
>>>> +
>>>> + return !be->users;
>>>
>>> Coverity CID 1531549 reports a concurrent data access violation because
>>> be->users is being accessed without holding the mutex.
>>>
>>> I wonder how useful is this mutex anyhow, since the code paths should
>>> be protected by the BQL lock. If you agree, I will send an update to
>>> simply drop be->lock and solve this report.
>> I am not totally comfortable with the fact BQL covers the same
>> protection? Please can you elaborate.
>
> These routines are called when a device is created which is called
> from the QEMU main thread which exits holding the BQL. It should be
> fine.
OK fine for me as well
Thanks
Eric
>
> Thanks,
>
> C.
>
>
