[Devel] [PATCH v7 1/2] overlayfs: add dynamic path resolving in mount options

[Pavel Tikhomirov]

On 6/3/20 7:58 PM, Alexander Mikhalitsyn wrote:
> This patch adds OVERLAY_FS_DYNAMIC_RESOLVE_PATH_OPTIONS
> compile-time option, and "dyn_path_opts" runtime module option.
> These options corresponds "dynamic path resolving in lowerdir,
> upperdir, workdir mount options" mode. If enabled, user may see
> real full paths relatively to the mount namespace in lowerdir,
> upperdir, workdir options (/proc/mounts, /proc/<fd>/mountinfo).
> 
> This patch is very helpful to checkpoint/restore functionality
> of overlayfs mounts. With this patch and CRIU it's real to C/R
> Docker containers with overlayfs storage driver.
> 
> Note: d_path function from dcache.c is used to resolve full path
> in mount namespace. This function also adds "(deleted)" suffix
> if dentry was deleted. So, If one of dentries in lowerdir, upperdir,
> workdir options is deleted, we will see "(deleted)" suffix in
> corresponding path.
> 
> https://jira.sw.ru/browse/PSBM-58614
> 
> Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn at virtuozzo.com>
> Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
> 
> v2: print_paths_option helper now uses only one additional page, code
> refactored according review comments
> 
> v3: print_paths_option helper now correctly escapes special symbols in
> paths, fixed ofs->lowerpaths[i] paths leak on error path in
> ovl_get_lowerstack function
> 
> v4: fixed ofs->lowerpaths[i] paths double put on error path in
> ovl_get_lowerstack function
> 
> v5: removed redundant path_put_init(&ofs->upperpath) in ovl_fill_super
> function
> 
> v6: print_path_option, print_paths_option reworked using seq_path() helper
> 
> v7: make rw access to dyn_path_opts parameter, 


> add explicit
> ofs->lowerpaths kfree and NULLify in ovl_get_lowerstack on error
> path to make code more readable

It looks like this also introduces null pointer dereference, and crash. 
Imagine:

ovl_fill_super {
   ovl_get_lowerstack {
     ovl_lower_dir
     numlower++ # numlower == 1
     ovl_get_lower_layers
       ofs->numlower++ # ofs->numlower == 1
     ovl_alloc_entry # return error
     goto out_err

   out_err:
     path_put_init(&ofs->lowerpaths[0])
     kfree(ofs->lowerpaths)
     ofs->lowerpaths = NULL
   } # return error
   goto out_err
out_err:
   ovl_free_fs
     path_put(&ofs->lowerpaths[0]) # ofs->lowerpaths == NULL
       dput(path->dentry) # crash
}

If you reset lowerpaths to NULL, you should check for NULL somethere in 
ovl_free_fs. The thing we should remember ofs->numlower is a counter for 
ofs->lower_layers it can be inconsistent with the state of ofs->lowerpaths.

I would suggest:

--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -259,9 +259,11 @@ static void ovl_free_fs(struct ovl_fs *ofs)
                 ovl_inuse_unlock(ofs->upper_mnt->mnt_root);
         mntput(ofs->upper_mnt);
         path_put(&ofs->upperpath);
-       for (i = 0; i < ofs->numlower; i++)
-               path_put(&ofs->lowerpaths[i]);
-       kfree(ofs->lowerpaths);
+       if (ofs->lowerpaths) {
+               for (i = 0; i < ofs->numlower; i++)
+                       path_put(&ofs->lowerpaths[i]);
+               kfree(ofs->lowerpaths);
+       }
         for (i = 0; i < ofs->numlower; i++)
                 mntput(ofs->lower_layers[i].mnt);
         for (i = 0; i < ofs->numlowerfs; i++)

> ---
>   fs/overlayfs/Kconfig     | 31 ++++++++++++++++++
>   fs/overlayfs/overlayfs.h |  4 +++
>   fs/overlayfs/ovl_entry.h |  6 ++--
>   fs/overlayfs/super.c     | 85 ++++++++++++++++++++++++++----------------------
>   fs/overlayfs/util.c      | 21 ++++++++++++
>   5 files changed, 107 insertions(+), 40 deletions(-)

> @@ -245,11 +249,15 @@ static void ovl_free_fs(struct ovl_fs *ofs)
>   	dput(ofs->indexdir);
>   	dput(ofs->workdir);
>   	if (ofs->workdir_locked)
> -		ovl_inuse_unlock(ofs->workbasedir);
> -	dput(ofs->workbasedir);
> +		ovl_inuse_unlock(ofs->workbasepath.dentry);
> +	path_put(&ofs->workbasepath);
>   	if (ofs->upperdir_locked)
>   		ovl_inuse_unlock(ofs->upper_mnt->mnt_root);
>   	mntput(ofs->upper_mnt);
> +	path_put(&ofs->upperpath);
> +	for (i = 0; i < ofs->numlower; i++)
> +		path_put(&ofs->lowerpaths[i]);
> +	kfree(ofs->lowerpaths);
>   	for (i = 0; i < ofs->numlower; i++)
>   		mntput(ofs->lower_layers[i].mnt);
>   	for (i = 0; i < ofs->numlowerfs; i++
> @@ -1358,21 +1369,21 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>   		sb->s_d_op = &ovl_dentry_operations.ops;
>   
>   out:
> -	for (i = 0; i < numlower; i++)
> -		path_put(&stack[i]);
> -	kfree(stack);
>   	kfree(lowertmp);
>   
>   	return oe;
>   
>   out_err:
> +	for (i = 0; i < numlower; i++)
> +		path_put_init(&ofs->lowerpaths[i]);
> +	kfree(ofs->lowerpaths);
> +	ofs->lowerpaths = NULL;
>   	oe = ERR_PTR(err);
>   	goto out;
>   }
>   
>   static int ovl_fill_super(struct super_block *sb, void *data, int silent)
>   {
> -	struct path upperpath = { };
>   	struct dentry *root_dentry;
>   	struct ovl_entry *oe;
>   	struct ovl_fs *ofs;


-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.