linux-kernel - Re: [PATCH 1/5] vfs, overlayfs, cachefiles: Turn I_OVL

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]

Date:   Mon, 31 Jan 2022 18:20:14 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     David Howells <dhowells@...hat.com>
Cc:     Miklos Szeredi <miklos@...redi.hu>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        linux-cachefs@...hat.com, Christoph Hellwig <hch@...radead.org>,
        Jeff Layton <jlayton@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] vfs, overlayfs, cachefiles: Turn I_OVL_INUSE into
 something generic

On Mon, Jan 31, 2022 at 5:32 PM David Howells <dhowells@...hat.com> wrote:
>
> Amir Goldstein <amir73il@...il.com> wrote:
>
> > Please leave ovl_* as wrappers instead of changing super.c.
>
> Do you want them turning into inline functions?
>

inline would be fine.

But I just noticed something that may wreck this party.

The assumption, when I proposed this merger, was that an inode for
upper/work and is exclusively owned by ovl driver, so there should be no
conflicts with other drivers setting inuse flag.

However, in ovl_check_layer(), we walk back to root to verify that an ovl
layer of a new instance is not a descendant of another ovl upper/work inuse.
So the meaning of I_OVL_INUSE is somewhat stronger than an exclusive inode lock.
It implies ownership on the entire subtree.

I guess there is no conflict with cachefiles since ovl upper/work should not be
intersecting with any cachefiles storage, but that complicates the
semantics when
it comes to a generic flag.

OTOH, I am not sure if this check on ovl mount is so smart to begin with.
This check was added (after the exclusive ownership meaning) to silence syzbot
that kept mutating to weird overlapping ovl setups.
I think that a better approach would be to fail a lookup in the upper layer
that results with a d_mountpoint() - those are not expected inside the overlay
upper layer AFAICT.

Anyway, I can make those changes if Miklos agrees with them, but I don't
want to complicate your work on this, so maybe for now, create the I_EXCL_INUSE
generic flag without changing overlayfs and I can make the cleanup to get rid of
I_OVL_INUSE later.

Thanks,
Amir.