答复: overlayfs issue: dir permission lost during overlayfs copy-up

["Lv Fei（吕飞）" <feilv@asrmicro.com>]

> -----邮件原件-----
> 发件人: Amir Goldstein [mailto:amir73il@gmail.com] 
> 发送时间: 2024年7月12日 17:41
> 收件人: Lv Fei（吕飞） <feilv@asrmicro.com>
> 抄送: miklos@szeredi.hu; overlayfs <linux-unionfs@vger.kernel.org>
> 主题: Re: overlayfs issue: dir permission lost during overlayfs copy-up
> 
> On Fri, Jul 12, 2024 at 7:18 AM Lv Fei（吕飞） <feilv@asrmicro.com> wrote:
> >
> >
> >
> > Dear Amir,
> >
> >
> >
> > Seems issue disappeared with below changes, can you help review below patch?
> >
> >
> >
> > Thank you!
> >
> >
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >
> > index 48bca5817f..e543b5563d 100644
> >
> > --- a/fs/overlayfs/copy_up.c
> >
> > +++ b/fs/overlayfs/copy_up.c
> >
> > @@ -851,9 +851,11 @@ static int ovl_copy_up_one(struct dentry *parent, 
> > struct dentry *dentry,
> >
> >
> >
> > int ovl_copy_up_flags(struct dentry *dentry, int flags)
> >
> > {
> >
> > +       struct super_block *sb = dentry->d_sb;
> >
> >         int err = 0;
> >
> >         const struct cred *old_cred;
> >
> >         bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED);
> >
> > +       unsigned int copies = 0;
> >
> >
> >
> >         /*
> >
> >          * With NFS export, copy up can get called for a disconnected non-dir.
> >
> > @@ -887,9 +889,14 @@ int ovl_copy_up_flags(struct dentry *dentry, int 
> > flags)
> >
> >
> >
> >                 dput(parent);
> >
> >                 dput(next);
> >
> > +
> >
> > +               copies++;
> >
> >         }
> >
> >         ovl_revert_creds(dentry->d_sb, old_cred);
> >
> >
> >
> > +       if (copies && d_is_dir(dentry) && sb->s_op->sync_fs)
> >
> > +               sb->s_op->sync_fs(sb, 1);
> >
> > +
> >
> 
> I am not sure if it is acceptable to add sync to parent dir copy up although this should be > relatively rare so maybe its fine??
> but if you do add sync you should be using fsync on the copied up parent directory - not ->sync_fs.
> 
> Anyway, this check is wrong.
> You should not be checking for d_is_dir(dentry), you should be checking if any *parents* were copied > up,
> 
> See more about this below...
> 
> >
> >
> >
> > 发件人: Lv Fei（吕飞）
> > 发送时间: 2024年7月12日 11:35
> > 收件人: 'amir73il@gmail.com' <amir73il@gmail.com>
> > 主题: overlayfs issue: dir permission lost during overlayfs copy-up
> >
> >
> >
> >
> >
> > Dear Amir,
> >
> >
> >
> > Sorry to bother you.
> >
> >
> >
> > Recently, we had a problem with overlayfs dir copy-up flow.
> >
> >
> >
> > Description:
> >
> > If a dir eyelyn/ exist in low layer, not exist in upper layer, after creating a new file(e.g. > eyelyn/ eyelyn.log) in this dir from overlayfs, permission of eyelyn/ may be abnormal after > power-cut.
> >
> > If add a sync after creating a new file, permission of eyelyn/ is always correct.
> >
> >
> >
> > Kernel Version:
> >
> > Linux OpenWrt 5.4.276+ #25 PREEMPT Fri Jul 12 02:21:17 UTC 2024 armv7l 
> > GNU/Linux
> >
> >
> >
> > Test Step:
> >
> > 1. mount –t squashfs /dev/mtdblock19 /system/etc
> >
> > root@OpenWrt:/system/etc# ls -l
> >
> > drwxr-xr-x    2 root     root             3 Jul 11  2024 eyelyn/
> >
> >
> >
> > 2. mount –t ubifs ubi0:etc /overlay/etc
> >
> > root@OpenWrt:/overlay/etc# ls -l
> >
> > drwxr-xr-x    8 root     root          1360 Jan  1 08:01 root/
> >
> > drwxr-xr-x    3 root     root           224 Jan  1 08:00 work/
> >
> > root@OpenWrt:/overlay/etc# ls -al root/
> >
> > drwxr-xr-x    8 root     root          1360 Jan  1 08:01 ./
> >
> > drwxr-xr-x    4 root     root           288 Jan  1 08:00 ../
> >
> >
> >
> > 3. mount –t overlay /system/etc -o 
> > noatime,lowerdir=/system/etc,upperdir=/overlay/etc/root,workdir=/overl
> > ay/etc/work
> >
> >
> >
> > 4. echo system > /system/etc /eyelyn/eyelyn.log
> >
> >
> >
> > 5. power cut
> >
> >
> >
> > 6. after next power on, sometimes dir eyelyn/ has wrong permission 
> > (d---------)
> >
> >
> >
> > mount –t ubifs ubi0:etc /overlay/etc
> >
> > root@OpenWrt:/overlay/etc# ls -l root/
> >
> > d---------   1 root     root           232 Jan  1 08:00 eyelyn
> >
> > root@OpenWrt:/overlay/etc# ls –l system/etc/eyelyn/eyelyn.log
> >
> > -rw-r--r--    1 root     root             0 Jan  1 08:00 /system/etc/eyelyn/eyelyn.log
> >
> >
> >
> > if we add sync to step 4, that is “echo system > /system/etc /eyelyn/eyelyn.log && sync”, then > everything is right.
> >
> >
> >
> > Do you have any suggestions?
> >
> >
> 
> 
> Overlayfs creates the upper dir in work directory, sets its metadata and only then moves it into > place, so the above is an "issue" with ubifs.
> 
> The thing about this "issue" is that the behavior that after move the old permissions cannot be > observed is not defined by POSIX, but it is the facto the behavior of most of the modern filesystems > (xfs, ext4 and most probably btrfs).
> 
> If you want to add a feature that adds fsync to copied up parent directories for filesystems like > ubifs that are not "strictly ordered metadata" then I think this needs to be an opt-in feature.
> 
> I must admit that this requirement from the upper fs is not documented and cannot be automatically > tested by overlayfs (fs do not advertise "strictly ordered metadata" property). It just happens to > be true for most of the common fs used as upper fs.
> 
> I wish we had called the mount option "volatile" "sync=none" and then we could have added > "sync=strict" for this and "sync=data" as the default.
> We can still do that and have "volatile" be an alias for "sync=none".
> 
> Thanks,
> Amir.

Very glad to receive your reply, Thank you for explanation.
As you suggested, I try to add mount option "sync=strict", change to use fsync for parent dir. Please help have a look.

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 48bca5817f..4258b8da8d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -851,6 +851,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 
 int ovl_copy_up_flags(struct dentry *dentry, int flags)
 {
+	struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 	int err = 0;
 	const struct cred *old_cred;
 	bool disconnected = (dentry->d_flags & DCACHE_DISCONNECTED);
@@ -884,6 +885,24 @@ int ovl_copy_up_flags(struct dentry *dentry, int flags)
 		}
 
 		err = ovl_copy_up_one(parent, next, flags);
+		if (ofs->config.volatile_sync && d_is_dir(next)) {
+			struct path upperpath;
+			struct file *new_file;
+
+			ovl_path_upper(next, &upperpath);
+
+			new_file = ovl_path_open(&upperpath,
+						O_LARGEFILE | O_WRONLY);
+			if (!IS_ERR(new_file)) {
+				if (ofs->config.volatile_sync ==
+				    OVL_VOLATILE_SYNC_DATA)
+					vfs_fsync(new_file, 1);
+				else
+					vfs_fsync(new_file, 0);
+
+				fput(new_file);
+			}
+		}
 
 		dput(parent);
 		dput(next);
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 2daba08f78..873d997fb9 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -5,6 +5,12 @@
  * Copyright (C) 2016 Red Hat, Inc.
  */
 
+enum {
+	OVL_VOLATILE_SYNC_NONE,
+	OVL_VOLATILE_SYNC_DATA,
+	OVL_VOLATILE_SYNC_STRICT,
+};
+
 struct ovl_config {
 	char *lowerdir;
 	char *upperdir;
@@ -18,6 +24,7 @@ struct ovl_config {
 	int xino;
 	bool metacopy;
 	bool override_creds;
+	int volatile_sync;
 };
 
 struct ovl_sb {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 093af1dcbd..68dee1850b 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -416,6 +416,9 @@ enum {
 	OPT_METACOPY_OFF,
 	OPT_OVERRIDE_CREDS_ON,
 	OPT_OVERRIDE_CREDS_OFF,
+	OPT_VOLATILE_SYNC_NONE,
+	OPT_VOLATILE_SYNC_DATA,
+	OPT_VOLATILE_SYNC_STRICT,
 	OPT_ERR,
 };
 
@@ -436,6 +439,9 @@ static const match_table_t ovl_tokens = {
 	{OPT_METACOPY_OFF,		"metacopy=off"},
 	{OPT_OVERRIDE_CREDS_ON,		"override_creds=on"},
 	{OPT_OVERRIDE_CREDS_OFF,	"override_creds=off"},
+	{OPT_VOLATILE_SYNC_NONE,	"sync=none"},
+	{OPT_VOLATILE_SYNC_DATA,	"sync=data"},
+	{OPT_VOLATILE_SYNC_STRICT,	"sync=strict"},
 	{OPT_ERR,			NULL}
 };
 
@@ -495,6 +501,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	if (!config->redirect_mode)
 		return -ENOMEM;
 	config->override_creds = ovl_override_creds_def;
+	config->volatile_sync = OVL_VOLATILE_SYNC_ DATA;
 
 	while ((p = ovl_next_opt(&opt)) != NULL) {
 		int token;
@@ -583,6 +590,18 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->override_creds = false;
 			break;
 
+		case OPT_VOLATILE_SYNC_NONE:
+			config->volatile_sync = OVL_VOLATILE_SYNC_NONE;
+			break;
+
+		case OPT_VOLATILE_SYNC_DATA:
+			config->volatile_sync = OVL_VOLATILE_SYNC_DATA;
+			break;
+
+		case OPT_VOLATILE_SYNC_STRICT:
+			config->volatile_sync = OVL_VOLATILE_SYNC_STRICT;
+			break;
+
 		default:
 			pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
 			return -EINVAL;

Thanks,
Fei