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