[lxc-devel] [PATCH] valgrind drd tool shows conflicting stores happening at lxc_global_config_value at src/lxc/utils.c

[Serge Hallyn]

Quoting S.Çağlar Onur (caglar at 10ur.org):
> Conflict occurs between following lines
> 
> [...]
> 269         if (values[i])
> 270                 return values[i];
> [...]
> 
> and
> 
> [...]
> 309         /* could not find value, use default */
> 310         values[i] = (*ptr)[1];
> [...]
> 
> so call it while holding the process_lock

Hmm.  The tip-off here that the patch is wrong is the fact that
you are taking the process_lock() away from the open and close
calls so that you can use them at callers of the callers.

You're very much right about the problem, though.  However, following
kernel style, let's instead add a lock for the static const values
inside lxc_global_config_value().  They are the problem, and they
should have their own lock.  I don't think using them is a particularly
hot point (not called all that often), so I don't think we need to
worry too much about using just the right type of lock.  So just
another pthread_mutex_t should be fine.

The process_lock() is very specifically meant to protect shared thread
values, like the file table.

If you're interested, I'll wait for you to send another patch.  If you
prefer that I do it, please let me know.

> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
> ---
>  src/lxc/cgroup.c |  2 +-
>  src/lxc/start.c  |  2 +-
>  src/lxc/utils.c  | 41 +++++++++++++++++++++++++++++++++--------
>  src/lxc/utils.h  |  3 ++-
>  4 files changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
> index 01ed040..1e1e72a 100644
> --- a/src/lxc/cgroup.c
> +++ b/src/lxc/cgroup.c
> @@ -91,7 +91,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta()
>  	int saved_errno;
>  
>  	errno = 0;
> -	cgroup_use = lxc_global_config_value("cgroup.use");
> +       cgroup_use = default_cgroup_use();
>  	if (!cgroup_use && errno != 0)
>  		return NULL;
>  	if (cgroup_use) {
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index 1cadc09..58e1194 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -695,7 +695,7 @@ int lxc_spawn(struct lxc_handler *handler)
>  	 * default value is available
>  	 */
>  	if (getuid() == 0)
> -		cgroup_pattern = lxc_global_config_value("cgroup.pattern");
> +               cgroup_pattern = default_cgroup_pattern();
>  	if (!cgroup_pattern)
>  		cgroup_pattern = "%n";
>  
> diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> index 9e2e326..6129cf8 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -269,9 +269,7 @@ const char *lxc_global_config_value(const char *option_name)
>  	if (values[i])
>  		return values[i];
>  
> -	process_lock();
>  	fin = fopen_cloexec(LXC_GLOBAL_CONF, "r");
> -	process_unlock();
>  	if (fin) {
>  		while (fgets(buf, 1024, fin)) {
>  			if (buf[0] == '#')
> @@ -317,30 +315,57 @@ const char *lxc_global_config_value(const char *option_name)
>  		errno = 0;
>  
>  out:
> -	process_lock();
>  	if (fin)
>  		fclose(fin);
> -	process_unlock();
>  	return values[i];
>  }
>  
>  const char *default_lvm_vg(void)
>  {
> -	return lxc_global_config_value("lvm_vg");
> +	process_lock();
> +	const char *ret = lxc_global_config_value("lvm_vg");
> +	process_unlock();
> +	return ret;
>  }
>  
>  const char *default_lvm_thin_pool(void)
>  {
> -	return lxc_global_config_value("lvm_thin_pool");
> +	process_lock();
> +	const char *ret = lxc_global_config_value("lvm_thin_pool");
> +	process_unlock();
> +	return ret;
>  }
>  
>  const char *default_zfs_root(void)
>  {
> -	return lxc_global_config_value("zfsroot");
> +	process_lock();
> +	const char *ret = lxc_global_config_value("zfsroot");
> +	process_unlock();
> +	return ret;
>  }
> +
>  const char *default_lxc_path(void)
>  {
> -	return lxc_global_config_value("lxcpath");
> +	process_lock();
> +	const char *ret = lxc_global_config_value("lxcpath");
> +	process_unlock();
> +	return ret;
> +}
> +
> +const char *default_cgroup_use(void)
> +{
> +	process_lock();
> +	const char *ret = lxc_global_config_value("cgroup.use");
> +	process_unlock();
> +	return ret;
> +}
> +
> +const char *default_cgroup_pattern(void)
> +{
> +	process_lock();
> +	const char *ret = lxc_global_config_value("cgroup.pattern");
> +	process_unlock();
> +	return ret;
>  }
>  
>  const char *get_rundir()
> diff --git a/src/lxc/utils.h b/src/lxc/utils.h
> index fc46760..8aa1550 100644
> --- a/src/lxc/utils.h
> +++ b/src/lxc/utils.h
> @@ -49,7 +49,8 @@ extern const char *default_lxc_path(void);
>  extern const char *default_zfs_root(void);
>  extern const char *default_lvm_vg(void);
>  extern const char *default_lvm_thin_pool(void);
> -
> +extern const char *default_cgroup_use(void);
> +extern const char *default_cgroup_pattern(void);
>  /* Define getline() if missing from the C library */
>  #ifndef HAVE_GETLINE
>  #ifdef HAVE_FGETLN
> -- 
> 1.8.3.2
> 
> 
> ------------------------------------------------------------------------------
> Android is increasing in popularity, but the open development platform that
> developers love is also attractive to malware creators. Download this white
> paper to learn more about secure code signing practices that can help keep
> Android apps secure.
> http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel