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: <CAOQ4uxiDokEQ0ZET+adP_CpvvTCRRLTcVb9d5mYAmM1q7y2PnQ@mail.gmail.com>
Date: Fri, 23 Aug 2024 13:42:37 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Fei Lv <feilv@...micro.com>
Cc: miklos@...redi.hu, 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 Fri, Aug 23, 2024 at 11:51 AM Amir Goldstein <amir73il@...il.com> wrote:
>
> On Mon, Jul 22, 2024 at 3:56 PM 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.
> >
> > As for timing, we are in the middle of the merge window for 6.11-rc1,
> > so we have some time before this can be considered for 6.12.
> > I will be on vacation for most of this development cycle, so either
> > Miklos will be able to queue it for 6.12 or I may be able to do
> > near the end of the 6.11 cycle.
> >
>
> Miklos,
>
> Please let me know what you think of this approach to handle ubifs upper.
> If you like it, I can queue this up for v6.12.
>
> Thanks,
> Amir.
>
> >
> > > ---
> > > V1 -> V2:
> > >  1. change open flags from "O_LARGEFILE | O_WRONLY" to "O_RDONLY".
> > >  2. change mount option to "fsync=ordered/strict/volatile".
> > >  3. ovl_should_sync_strict() implies ovl_should_sync().
> > >  4. remove redundant ovl_should_sync_strict from ovl_copy_up_meta_inode_data.
> > >  5. update commit log.
> > >  6. update documentation overlayfs.rst.
> > >

Hi Fei,

I started to test this patch and it occured to me that we have no test
coverage for
the "volatile" feature.

Filesystem durability tests are not easy to write and I know that you
tested your
own use case, so I will not ask you to write a regression test as a
condition for merge,
but if you are willing to help, it would be very nice to add this test coverage.

There is already one overlayfs test in fstests (overlay/078) which
tests behavior
of overlayfs copy up during power cut (a.k.a shutdown).

One thing that I do request is that you confirm that you tested that the legacy
"volatile" mount option still works as before.
I saw that you took care of preserving the legacy mount option in display,
which is good practice.

Thanks,
Amir.