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: <CAOQ4uxiYGsKzMZ73=WLZqseU=ibboFtPfqpeGtmFWYY3uxjMvw@mail.gmail.com>
Date: Tue, 27 Aug 2024 10:46:47 +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 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?
>

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.

Thanks,
Amir.