linux-kernel - Re: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper

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: <YKU05k0P7YjH/g6E@zeniv-ca.linux.org.uk>
Date:   Wed, 19 May 2021 15:55:18 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Jia He <justin.he@....com>, Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Jonathan Corbet <corbet@....net>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Eric Biggers <ebiggers@...gle.com>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-s390 <linux-s390@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a
 new helper

On Wed, May 19, 2021 at 11:07:04AM +0300, Andy Shevchenko wrote:
> On Wed, May 19, 2021 at 12:48:59AM +0000, Al Viro wrote:
> > ... and leave the rename_lock/mount_lock handling in prepend_path()
> > itself
> 
> ...
> 
> > +			if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
> > +				return 1;	// absolute root
> > +			else
> > +				return 2;	// detached or not attached yet
> 
> Would it be slightly better to read
> 
> 			if (IS_ERR_OR_NULL(mnt_ns) || is_anon_ns(mnt_ns))
> 				return 2;	// detached or not attached yet
> 			else
> 				return 1;	// absolute root
> 
> ?
> 
> Oh, I have noticed that it's in the original piece of code (perhaps separate
> change if we ever need it?).

The real readability problem here is not the negations.  There are 4 possible
states for vfsmount encoded via ->mnt_ns:
	1) not attached to any tree, kept alive by refcount alone.
->mnt_ns == NULL.
	2) long-term unattached.  Not a part of any mount tree, but we have
a known holder for it and until that's gone (making ->mnt_ns NULL), refcount
is guaranteed to remain positive.  pipe_mnt is an example of such.
->mnt_ns == MNT_NS_INTERNAL, which is encoded as ERR_PTR(-1), thus the use of
IS_ERR_OR_NULL here (something I'd normally taken out and shot - use of that
primitive is a sign of lousy API or of a cargo-culted "defensive programming").
	3) part of a temporary mount tree; not in anyone's namespace.
->mnt_ns points the tree in question, ->mnt_ns->seq == 0.
	4) belongs to someone's namespace.  ->mnt_ns points to that,
->mnt_ns->seq != 0.  That's what we are looking for here.

	It's kludges all the way down ;-/  Note that temporary tree can't become
a normal one or vice versa - mounts can get transferred to normal namespace,
but they will see ->mnt_ns reassigned to that.  IOW, ->mnt_ns->seq can't
get changed without a change to ->mnt_ns.  I suspect that the right way
to handle that would be to have that state stored as explicit flags.

	All mounts are created (and destroyed) in state (1); state changes:
commit_tree() - (1) or (3) to (3) or (4)
umount_tree() - (3) or (4) to (1)
clone_private_mount() - (1) to (2)
open_detached_copy() - (1) to (3)
copy_mnt_ns() - (1) to (4)
mount_subtree() - (1) to (3)
fsmount() - (1) to (3)
init_mount_tree() - (1) to (4)
kern_mount() - (1) to (2)
kern_unmount{,_array}() - (2) to (1)

	commit_tree() has a pathological call chain that has it
attach stuff to temporary tree; that's basically automount by lookup in
temporary namespace.  It can distinguish it from the usual (adding to
normal namespace) by looking at the state of mountpoint we are attaching
to - or simply describe all cases as "(1) or (3) to whatever state the
mountpoint is".

	One really hot path where we check (1) vs. (2,3,4) is
mntput_no_expire(), which is the initial reason behind the current
representation.  However, read from ->mnt_flags is just as cheap as
that from ->mnt_ns and the same reasons that make READ_ONCE()
legitimate there would apply to ->mnt_flags as well.

	We can't reuse MNT_INTERNAL for that, more's the pity -
it's used to mark the mounts (kern_mount()-created, mostly) that
need to destroyed synchronously on the final mntput(), with no
task_work_add() allowed (think of module_init() failing halfway through,
with kern_unmount() done to destroy the internal mounts already created;
we *really* don't want to delay that filesystem shutdown until insmod(2)
heads out to userland).  Another headache is in LSM shite, as usual...

	Anyway, sorting that out is definitely a separate story.