[libvirt] [PATCH 16/17] nwfilter: Remove recursive locking for nwfilter instantiation

[Michal Privoznik]

On 06/02/2017 12:25 PM, John Ferlan wrote:
> The current algorithm required usage of recursive locks because there
> was no other mechanism available to ensure that the object wasn't deleted
> whilst the instantiation was taking place.
> 
> Now that we have object references, lets use those to ensure the object
> isn't deleted whilst we're working through the instantiation thus removing
> the need for recursive locks.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/virnwfilterobj.c              | 13 +++++++++--
>  src/nwfilter/nwfilter_gentech_driver.c | 40 +++++++++++++++++++---------------
>  2 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 84ef7d1..ea1323d 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -303,13 +303,22 @@ virNWFilterObjListFindByName(virNWFilterObjListPtr nwfilters,
>  }
>  
>  
> +/**
> + * To avoid the need to have recursive locks as a result of how the
> + * virNWFilterInstantiateFilter processing works, this API will not
> + * lock the returned object, instead just increase the refcnt of the
> + * object to the caller to allow the caller to handle.
> + */
>  virNWFilterObjPtr
>  virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
>                                          const char *filtername)
>  {
>      virNWFilterObjPtr obj;
>  
> -    if (!(obj = virNWFilterObjListFindByName(nwfilters, filtername))) {
> +    virObjectLock(nwfilters);
> +    obj = virNWFilterObjListFindByNameLocked(nwfilters, filtername);
> +    virObjectUnlock(nwfilters);
> +    if (!obj) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("referenced filter '%s' is missing"), filtername);
>          return NULL;
> @@ -318,7 +327,7 @@ virNWFilterObjListFindInstantiateFilter(virNWFilterObjListPtr nwfilters,
>      if (virNWFilterObjWantRemoved(obj)) {
>          virReportError(VIR_ERR_NO_NWFILTER,
>                         _("Filter '%s' is in use."), filtername);
> -        virNWFilterObjEndAPI(&obj);
> +        virNWFilterObjUnref(obj);
>          return NULL;
>      }

So now an unlocked obj is returned. This feels wrong ... So for
instance: virNWFilterIncludeDefToRuleInst() calls
virNWFilterObjListFindInstantiateFilter(). So it obtains ref'd but
unlocked object. Then it calls virNWFilterObjGetDef() over it. Well, we
are reading without lock, so we might as well be accessing stale
pointer. For instance the following might happen:

1) threadA is in virNWFilterIncludeDefToRuleInst() and calls
virNWFilterObjGetDef(). It gets pointer to current definition of nwfilter.

2) threadB starts to update the definition of the object. Since the
object is not locked, nothing stops it from doing so. As part of the
process, current definition is freed.

3) threadA touches freed data.

Michal