linux-kernel - Re: [PATCH V2] ovl: fsync after metadata copy-up via mount option "fsync=strict"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxi-BuKU-AbyydVB2c8z0DiPP-Ednu+bN3JB2Cqf7rZamA@mail.gmail.com>
Date: Thu, 29 Aug 2024 12:29:43 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Miklos Szeredi <miklos@...redi.hu>
Cc: Fei Lv <feilv@...micro.com>, linux-unionfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, lianghuxu@...micro.com
Subject: Re: [PATCH V2] ovl: fsync after metadata copy-up via mount option "fsync=strict"

On Tue, Aug 27, 2024 at 10:46 AM Amir Goldstein <amir73il@...il.com> wrote:
>
> On Mon, Aug 26, 2024 at 5:59 PM Miklos Szeredi <miklos@...redi.hu> wrote:
> >
> > On Mon, 22 Jul 2024 at 15:56, Amir Goldstein <amir73il@...il.com> wrote:
> > >
> > > On Mon, Jul 22, 2024 at 1:14 PM Fei Lv <feilv@...micro.com> wrote:
> > > >
> > > > For upper filesystem which does not enforce ordering on storing of
> > > > metadata changes(e.g. ubifs), when overlayfs file is modified for
> > > > the first time, copy up will create a copy of the lower file and
> > > > its parent directories in the upper layer. Permission lost of the
> > > > new upper parent directory was observed during power-cut stress test.
> > > >
> > > > Fix by adding new mount opion "fsync=strict", make sure data/metadata of
> > > > copied up directory written to disk before renaming from tmp to final
> > > > destination.
> > > >
> > > > Signed-off-by: Fei Lv <feilv@...micro.com>
> > >
> > > Reviewed-by: Amir Goldstein <amir73il@...il.com>
> > >
> > > but I'd also like to wait for an ACK from Miklos on this feature.
> >
> > I'm okay with this.  I'm a little confused about sync=strict mode,
> > since most copy ups will have vfs_fsync() called twice.  Is this what
> > we want, or could this be consolidated into a single fsync?
> >
>
> Maybe it could, but remember that ubifs strict mode is the odd case
> if we have an extra fsync for the odd case, I think code simplicity is
> a more important factor.
>
> > Also is it worth optimizing away the fsync on the directory in cases
> > the filesystem is well behaved?  Maybe we should just move the
> > vfs_fsync() call into ovl_copy_up_metadata() and omit the complexity
> > related to the additional mount option?
> >

Maybe, but note that in ovl_copy_up_meta_inode_data(),
copy up of data still requires fsync and there is no call to
ovl_copy_up_metadata() in that code path, so trying to optimize
double fsync in all the code paths in going to be a PITA IMA
and not worth the trouble.

>
> Hmm. Maybe you are confused by the commit message that only mentions
> fsync of the parent directory (same as the reported reproducer), but
> the strict mode fsync also affects metacopy, not only parent dir copy up.
>
> > To me it feels that it shouldn't matter in terms of performance, but
> > if reports of performance regressions come in, we can still make this
> > optional.
> >
>
> I think that the case of chown -R with metacopy is going to be terribly crippled
> if every metacopy gets and fsync.
>

But maybe we can ignore crash safety of metacopy on ubifs, because
1. the ubifs users may not be using this feature
2. ubifs may be nice and takes care of ordering O_TMPFILE
    metadata updates before exposing the link

Then we can do the following:
IF (metacopy_enabled)
    fsync only in ovl_copy_up_file()
ELSE
    fsync only in ovl_copy_up_metadata()

Let me know what you think.

Thanks,
Amir.