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 <[email protected]>
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.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
