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

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

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