On Tue, 24 Mar 2020 at 13:18, Marcel Apfelbaum <marcel.apfelb...@gmail.com>
wrote:
> Hi Peter,Yuval
>
> On 3/24/20 1:05 PM, Peter Maydell wrote:
> > On Tue, 24 Mar 2020 at 10:54, Yuval Shaia <yuval.shaia...@gmail.com>
> wrote:
> >> To protect from the case that users of the protected_qlist are still
> >> using the qlist let's lock before detsroying it.
> >>
> >> Reported-by: Coverity (CID 1421951)
> >> Signed-off-by: Yuval Shaia <yuval.shaia...@gmail.com>
> >> ---
> >> hw/rdma/rdma_utils.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
> >> index 73f279104c..55779cbef6 100644
> >> --- a/hw/rdma/rdma_utils.c
> >> +++ b/hw/rdma/rdma_utils.c
> >> @@ -63,6 +63,7 @@ void rdma_protected_qlist_init(RdmaProtectedQList
> *list)
> >> void rdma_protected_qlist_destroy(RdmaProtectedQList *list)
> >> {
> >> if (list->list) {
> >> + qemu_mutex_lock(&list->lock);
> >> qlist_destroy_obj(QOBJECT(list->list));
> >> qemu_mutex_destroy(&list->lock);
> >> list->list = NULL;
> > Looking at the code a bit more closely, I don't think that taking
> > the lock here helps. If there really could be another thread
> > that might be calling eg rdma_protected_qlist_append_int64()
> > at the same time that we're calling rdma_protected_qlist_destroy()
> > then it's just as likely that the code calling destroy could
> > finish just before the append starts: in that case the append
> > will crash because the list has been freed and the mutex destroyed.
> >
> > So I think we require that the user of a protected-qlist
> > ensures that there are no more users of it before it is
> > destroyed (which is fairly normal semantics), and the code
> > as it stands is correct (ie coverity false-positive).
>
> I agree is a false positive for our case.
> "rdma_protected_qlist_destroy" is called by "mad_fini" which in turn is
> called by "rdma_backend_fini"
> *after* the VM shutdown, at this point there is no active lock user.
>
As i already said, current code makes sure it will not happen however it
better that API will ensure this and will not trust callers.
>
> Thanks,
> Marcel
>
> > thanks
> > -- PMM
>
>
