hw/rdma: Lock before destroy

[PATCH] hw/rdma: Lock before destroy

Posted by Yuval Shaia 4 years, 1 month ago

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.ml@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;
-- 
2.25.1

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Peter Maydell 4 years, 1 month ago

On Tue, 24 Mar 2020 at 10:54, Yuval Shaia <yuval.shaia.ml@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.ml@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).

thanks
-- PMM

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Marcel Apfelbaum 4 years, 1 month ago

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

Thanks,
Marcel

> thanks
> -- PMM

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Peter Maydell 4 years, 1 month ago

On Tue, 24 Mar 2020 at 11:18, Marcel Apfelbaum
<marcel.apfelbaum@gmail.com> wrote:
>
> Hi Peter,Yuval
>
> On 3/24/20 1:05 PM, Peter Maydell wrote:
> > 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.

Also, the function coverity queried was rdma_protected_gslist_destroy(),
not rdma_protected_qlist_destroy().

I notice that the gslist_destroy function does not destroy
the mutex -- is this an intentional difference ?

thanks
-- PMM

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Yuval Shaia 4 years, 1 month ago

On Tue, 24 Mar 2020 at 13:25, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 24 Mar 2020 at 11:18, Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com> wrote:
> >
> > Hi Peter,Yuval
> >
> > On 3/24/20 1:05 PM, Peter Maydell wrote:
> > > 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.
>
> Also, the function coverity queried was rdma_protected_gslist_destroy(),
> not rdma_protected_qlist_destroy().
>

Yeah but pattern is the same, if we agree to threat it as false-positive
then it applies to the two cases.


>
> I notice that the gslist_destroy function does not destroy
> the mutex -- is this an intentional difference ?
>

No - it is a bug!


>
> thanks
> -- PMM
>

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Yuval Shaia 4 years, 1 month ago

On Tue, 24 Mar 2020 at 13:18, Marcel Apfelbaum <marcel.apfelbaum@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.ml@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.ml@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
>
>

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Peter Maydell 4 years, 1 month ago

On Tue, 24 Mar 2020 at 11:25, Yuval Shaia <yuval.shaia.ml@gmail.com> wrote:
> 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.

I agree with the principle, but I think that here there is no
way to do it -- if you are literally destroying an object
then it is invalid to use it after destruction and there
is no way to have a lock that protects against that kind
of bug, unless the lock is at a higher level (ie the
thing that owns the destroyed-object has a lock and
doesn't allow access to it outside the lock or without
a check for has-been-destroyed). Just throwing an extra
mutex-lock into the destroy function doesn't add any
protection.

thanks
-- PMM

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Yuval Shaia 4 years, 1 month ago

On Tue, 24 Mar 2020 at 13:55, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 24 Mar 2020 at 11:25, Yuval Shaia <yuval.shaia.ml@gmail.com>
> wrote:
> > 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.
>
> I agree with the principle, but I think that here there is no
> way to do it -- if you are literally destroying an object
> then it is invalid to use it after destruction and there
> is no way to have a lock that protects against that kind
> of bug, unless the lock is at a higher level (ie the
> thing that owns the destroyed-object has a lock and
> doesn't allow access to it outside the lock or without
> a check for has-been-destroyed). Just throwing an extra
> mutex-lock into the destroy function doesn't add any
> protection.
>

Make sense.
So what i can do is to check list->list at every API since destroy
functions sets it to NULL.

Does it make sense?


>
> thanks
> -- PMM
>

Re: [PATCH] hw/rdma: Lock before destroy

Posted by Peter Maydell 4 years, 1 month ago

On Tue, 24 Mar 2020 at 12:57, Yuval Shaia <yuval.shaia.ml@gmail.com> wrote:
> So what i can do is to check list->list at every API since destroy
> functions sets it to NULL.

No, that won't help. You can't check list->list for NULL without
taking the lock (because in the append etc functions you really
could be multithreaded), and you can't take the lock because
it might have been destroyed.

thanks
-- PMM