Re: [XEN][RFC PATCH v4 11/16] common/device_tree: Add rwlock for dt_host

[Julien Grall <julien@xxxxxxx>]

Hi Vikram,

On 07/12/2022 06:18, Vikram Garhwal wrote:
>   Dynamic programming ops will modify the dt_host and there might be other
>   function which are browsing the dt_host at the same time. To avoid the race
>   conditions, adding rwlock for browsing the dt_host.
Looking at the user below, it is not entirely clear what the lock is 
actually protecting. For instance...
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> ---
>   xen/common/device_tree.c      | 27 +++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h |  6 ++++++
>   2 files changed, 33 insertions(+)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index acf26a411d..51ee2a5edf 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const struct 
> dt_device_node *np,
>       if ( !np )
>           return NULL;
>   
> +    read_lock(&dt_host->lock);
> +
>       for ( pp = np->properties; pp; pp = pp->next )
>       {
>           if ( dt_prop_cmp(pp->name, name) == 0 )
> @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const struct 
> dt_device_node *np,
>           }
>       }
>   
> +    read_unlock(&dt_host->lock);
>       return pp;
>   }
>   
> @@ -336,11 +339,14 @@ struct dt_device_node *dt_find_node_by_name(struct dt_device_node *from,
>       struct dt_device_node *np;
>       struct dt_device_node *dt;
>   
> +    read_lock(&dt_host->lock);
> +
>       dt = from ? from->allnext : dt_host;
>       dt_for_each_device_node(dt, np)
>           if ( np->name && (dt_node_cmp(np->name, name) == 0) )
>               break;
>   
> +    read_unlock(&dt_host->lock);
>       return np;
... I was expecting the read lock to also protect the value returned 
from being freed. But this is not the case.
Cheers,

--
Julien Grall