linux-kernel - Re: [RFC PATCH v5 06/10] ovl: implement overlayfs' ->write

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] [day] [month] [year] [list]

Date:   Sat, 5 Feb 2022 18:23:32 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     Chengguang Xu <cgxu519@...ernel.net>
Cc:     Jan Kara <jack@...e.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        ronyjin <ronyjin@...cent.com>,
        charliecgxu <charliecgxu@...cent.com>,
        Vivek Goyal <vgoyal@...hat.com>,
        Miklos Szeredi <miklos@...redi.hu>
Subject: Re: [RFC PATCH v5 06/10] ovl: implement overlayfs' ->write_inode operation

On Sat, Feb 5, 2022 at 6:10 PM Chengguang Xu <cgxu519@...ernel.net> wrote:
>
> 在 2021/12/7 13:33, Amir Goldstein 写道:
> > On Sun, Dec 5, 2021 at 4:07 PM Chengguang Xu <cgxu519@...ernel.net> wrote:
> >>   ---- 在 星期四, 2021-12-02 06:47:25 Amir Goldstein <amir73il@...il.com> 撰写 ----
> >>   > On Wed, Dec 1, 2021 at 6:24 PM Chengguang Xu <cgxu519@...ernel.net> wrote:
> >>   > >
> >>   > >  ---- 在 星期三, 2021-12-01 21:46:10 Jan Kara <jack@...e.cz> 撰写 ----
> >>   > >  > On Wed 01-12-21 09:19:17, Amir Goldstein wrote:
> >>   > >  > > On Wed, Dec 1, 2021 at 8:31 AM Chengguang Xu <cgxu519@...ernel.net> wrote:
> >>   > >  > > > So the final solution to handle all the concerns looks like accurately
> >>   > >  > > > mark overlay inode diry on modification and re-mark dirty only for
> >>   > >  > > > mmaped file in ->write_inode().
> >>   > >  > > >
> >>   > >  > > > Hi Miklos, Jan
> >>   > >  > > >
> >>   > >  > > > Will you agree with new proposal above?
> >>   > >  > > >
> >>   > >  > >
> >>   > >  > > Maybe you can still pull off a simpler version by remarking dirty only
> >>   > >  > > writably mmapped upper AND inode_is_open_for_write(upper)?
> >>   > >  >
> >>   > >  > Well, if inode is writeably mapped, it must be also open for write, doesn't
> >>   > >  > it? The VMA of the mapping will hold file open. So remarking overlay inode
> >>   > >  > dirty during writeback while inode_is_open_for_write(upper) looks like
> >>   > >  > reasonably easy and presumably there won't be that many inodes open for
> >>   > >  > writing for this to become big overhead?
> >>   >
> >>   > I think it should be ok and a good tradeoff of complexity vs. performance.
> >>
> >> IMO, mark dirtiness on write is relatively simple, so I think we can mark the
> >> overlayfs inode dirty during real write behavior and only remark writable mmap
> >> unconditionally in ->write_inode().
> >>
> > If by "on write" you mean on write/copy_file_range/splice_write/...
> > then yes I agree
> > since we have to cover all other mnt_want_write() cases anyway.
> >
> >>   >
> >>   > >  >
> >>   > >  > > If I am not mistaken, if you always mark overlay inode dirty on ovl_flush()
> >>   > >  > > of FMODE_WRITE file, there is nothing that can make upper inode dirty
> >>   > >  > > after last close (if upper is not mmaped), so one more inode sync should
> >>   > >  > > be enough. No?
> >>   > >  >
> >>   > >  > But we still need to catch other dirtying events like timestamp updates,
> >>   > >  > truncate(2) etc. to mark overlay inode dirty. Not sure how reliably that
> >>   > >  > can be done...
> >>   > >  >
> >>   >
> >>   > Oh yeh, we have those as well :)
> >>   > All those cases should be covered by ovl_copyattr() that updates the
> >>   > ovl inode ctime/mtime, so always dirty in ovl_copyattr() should be good.
> >>
> >> Currently ovl_copyattr() does not cover all the cases, so I think we still need to carefully
> >> check all the places of calling mnt_want_write().
> >>
> > Careful audit is always good, but if we do not have ovl_copyattr() in
> > a call site
> > that should mark inode dirty, then it sounds like a bug, because ovl inode ctime
> > will not get updated. Do you know of any such cases?
>
> Sorry for my late response, I've been very busy lately.
> For your question, for example, there is a case of calling
> ovl_want_write() in ovl_cache_get_impure() and caller does not call
> ovl_copyattr()
> so I think we should explicitly mark ovl inode dirty in that case. Is
> that probably a bug?
>
>

The correct behavior would be similar to that of setting impure xattr
in ovl_link_up().
We would want to snapshot the upperdir attrs before removing xattr
and restore them after (best effort).
Not that this case is so important, but if you have an opportunity
to mark inode dirty in ovl_copyattr() I think that would be the best
way to go.

Thanks,
Amir.