On Mon, Jul 15, 2024 at 9:07 AM Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> wrote: > > > > > -----邮件原件----- > > 发件人: Amir Goldstein [mailto:amir73il@xxxxxxxxx] > > 发送时间: 2024年7月12日 17:41 > > 收件人: Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> > > 抄送: miklos@xxxxxxxxxx; overlayfs <linux-unionfs@xxxxxxxxxxxxxxx> > > 主题: Re: overlayfs issue: dir permission lost during overlayfs copy-up > > > > On Fri, Jul 12, 2024 at 7:18 AM Lv Fei（吕飞） <feilv@xxxxxxxxxxxx> 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@xxxxxxxxx' <amir73il@xxxxxxxxx> > > > 主题: 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,
