Re: overlayfs issue: dir permission lost during overlayfs copy-up

Re: overlayfs issue: dir permission lost during overlayfs copy-up

[Amir Goldstein]

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=/overlay/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.

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

[Lv Fei（吕飞）]

> -----邮件原件-----
> 发件人: 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

Re: overlayfs issue: dir permission lost during overlayfs copy-up

[Amir Goldstein]

On Mon, Jul 15, 2024 at 9:07 AM Lv Fei（吕飞） <feilv@asrmicro.com> wrote:
>
>
>
> > -----邮件原件-----
> > 发件人: 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.
>

Ok, but if you want to submit this change please follow
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
See comments below

> 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;

ofs = OVL_FS(dentry->d_sb);

>         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);

Not needed already done in ovl_copy_up_file()

> +                               else
> +                                       vfs_fsync(new_file, 0);
> +
> +                               fput(new_file);
> +                       }
> +               }

Not the right place for fsync.
This should be at the end of ovl_copy_up_metadata()
similar to fsync at the end of ovl_copy_up_file().
The check for sync mode should be abstracted by the helper
like ovl_should_sync(), maybe ovl_should_sync_strict()

>
>                 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;

This word volatile_ is unneeded and wrong. "volatile" means "no sync"
Please *replace the config ovl_volatile with sync_mode, don't keep both
and grep for all access to ovl_volatile to replace them with adjusted
sync_mode code.

>  };
>
>  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,

This word _VOLATILE_ is unneeded and wrong. "volatile" means "no sync"
Which version are you basing your patch on?
You should be developing on top of current upstream kernel, where this
parsing code is at params.c.

You should follow the example of ovl_parameter_verity()
which accepts enum values {off, on, require} which is quite the
same as what you want for sync mode.

>         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"},

Note that you need to add the new sync option AND keep the legacy
"volatile" mount option.

>         {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;
> +


And effectively the two mount options to do the same, i.e.:

case Opt_sync:
                config->sync_mode = result.uint_32;
                break;
case Opt_volatile:
                config->sync_mode = OVL_SYNC_OFF;
                break;

>                 default:
>                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
>                         return -EINVAL;
>

Also need to display the mode in ovl_show_options().

Thanks,
Amir,

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

[Lv Fei（吕飞）]

> -----邮件原件-----
> 发件人: Amir Goldstein [mailto:amir73il@gmail.com] 
> 发送时间: 2024年7月15日 18:08
> 收件人: 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 Mon, Jul 15, 2024 at 9:07 AM Lv Fei（吕飞） <feilv@asrmicro.com> wrote:
> >
> >
> >
> > > -----邮件原件-----
> > > 发件人: 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=/o
> > > > verl
> > > > 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.
> >
> 
> Ok, but if you want to submit this change please follow https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Thanks, I will learn the submit flow and have a try.

> See comments below
> 
> > 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;
> 
> ofs = OVL_FS(dentry->d_sb);
> 
> >         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);
> 
> Not needed already done in ovl_copy_up_file()
> 
> > +                               else
> > +                                       vfs_fsync(new_file, 0);
> > +
> > +                               fput(new_file);
> > +                       }
> > +               }
> 
> Not the right place for fsync.
> This should be at the end of ovl_copy_up_metadata() similar to fsync at the end of ovl_copy_up_file().
> The check for sync mode should be abstracted by the helper like ovl_should_sync(), maybe ovl_should_sync_strict()
> 
> >
> >                 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;
> 
> This word volatile_ is unneeded and wrong. "volatile" means "no sync"
> Please *replace the config ovl_volatile with sync_mode, don't keep both and grep for all access to ovl_volatile to replace them with adjusted sync_mode code.
> 
> >  };
> >
> >  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,
> 
> This word _VOLATILE_ is unneeded and wrong. "volatile" means "no sync"
> Which version are you basing your patch on?
> You should be developing on top of current upstream kernel, where this parsing code is at params.c.
> 
> You should follow the example of ovl_parameter_verity() which accepts enum values {off, on, require} which is quite the same as what you want for sync mode.

Sorry, kernel version I am working on is 5.4.276. Seems there is much difference.

> 
> >         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"},
> 
> Note that you need to add the new sync option AND keep the legacy "volatile" mount option.
> 
> >         {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;
> > +
> 
> 
> And effectively the two mount options to do the same, i.e.:
> 
> case Opt_sync:
>                 config->sync_mode = result.uint_32;
>                 break;
> case Opt_volatile:
>                 config->sync_mode = OVL_SYNC_OFF;
>                 break;
> 
> >                 default:
> >                         pr_err("overlayfs: unrecognized mount option \"%s\" or missing value\n", p);
> >                         return -EINVAL;
> >
> 
> Also need to display the mode in ovl_show_options().
> 
> Thanks,
> Amir,

After reading latest code, found the mount option "volatile", now I understand what you mean
by "sync=none"/"sync=data"/"sync=strict", "sync=data" is default mount option.
what I need to do is extending the meaning of config->ovl_volatile(used to control data sync), using config->sync_mode instead.

And for version 5.4.276, I need to add fsync at the end of ovl_copy_up_inode (correspond to latest function ovl_copy_up_metadata), right?

Thank you for your patience!

Fei

Re: overlayfs issue: dir permission lost during overlayfs copy-up

[Amir Goldstein]

> And for version 5.4.276, I need to add fsync at the end of ovl_copy_up_inode (correspond to latest function ovl_copy_up_metadata), right?
>

Sounds right, but I do not have time to examine out-of-tree backport effort.
First, you need to provide a working and tested patch for upstream.
If that gets accepted, we can discuss backporting efforts.
But the general idea is this:
ovl_do_rename() serves as the "atomic" copy up operation
everything that happens before that will not be observed after crash
if rename did not happen.

The problem is that inode metadata changes that happened before rename
are NOT guaranteed by POSIX to be observed after crash even by rename
is observe unless fsync() is called on the source inode before rename.

Thanks,
Amir.

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

[Lv Fei（吕飞）]

> -----邮件原件-----
> 发件人: Amir Goldstein [mailto:amir73il@gmail.com] 
> 发送时间: 2024年7月16日 16:19
> 收件人: Lv Fei（吕飞） <feilv@asrmicro.com>
> 抄送: miklos@szeredi.hu; overlayfs <linux-unionfs@vger.kernel.org>
> 主题: Re: overlayfs issue: dir permission lost during overlayfs copy-up
> 
> > And for version 5.4.276, I need to add fsync at the end of ovl_copy_up_inode (correspond to latest function ovl_copy_up_metadata), right?
> >
> 
> Sounds right, but I do not have time to examine out-of-tree backport effort.
> First, you need to provide a working and tested patch for upstream.
> If that gets accepted, we can discuss backporting efforts.
> But the general idea is this:
> ovl_do_rename() serves as the "atomic" copy up operation everything that happens before that will not be observed after crash if rename did not happen.
> 
> The problem is that inode metadata changes that happened before rename are NOT guaranteed by POSIX to be observed after crash even by rename is observe unless fsync() is called on the source inode before rename.
> 
> Thanks,
> Amir.

OK, I finished a patch based on 6.10 today, will post for review.

Thanks,
Fei