Re: [PULL 20/47] backends/iommufd: Introduce the iommufd object

[Eric Auger <eric.auger@redhat.com>]

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