Re: [PATCH v2] namespace: Added pointer check in copy_mnt_ns()

[Al Viro]

On Fri, Nov 18, 2022 at 02:41:37PM +0300, Denis Arefev wrote:
> Return value of a function 'next_mnt' is dereferenced at
> namespace.c:3377 without checking for null,
> but it is usually checked for this function
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

NAK.

I see a bug in there, but it's not going to trigger a NULL pointer
dereference and your patch doesn't fix it at all.

That loop ought to be
		// skipped mntns binding?
                while (p->mnt.mnt_root != q->mnt.mnt_root)
			p = next_mnt(skip_mnt_tree(p), old);

and I suspect that it'll confuse your tool even worse.

What happens here is that new tree is congruent to the old one,
with some subtrees skipped.  Each node N in the new tree is a
clone of some node (Origin(N)) in the old one.  Copying preserves
node order.  We want to have p == Origin(q) on each iteration.

What we really have (due to the real bug) is

	p is no later than Origin(q) in node ordering

Initially it's trivially true (p points to root of the old tree,
and the only way it would *not* be copied would be to somehow get
mntns binding as root; in that case copy_tree() would've failed
and we wouldn't get to that loop at all).

Suppose it is true on some iteration.  What happens on the next
one?  q hadn't been the last node in the new tree, or we would've
found next_mnt(q, new) to be NULL and exited the loop.  But
that means that
	p "<=" Origin(q) "<" Origin(next_mnt(q, new))
("<" and "<=" in the node ordering, that is).  So p couldn't
have been the last node in the old tree and
	next_mnt(p, old) "<=" Origin(next_mnt(q, new))
After the
                p = next_mnt(p, old);
		q = next_mnt(q, new);
		if (!q)
			break;
we have
	p != NULL && p "<=" Origin(q)

Cloning preserves ->mnt_root, so the subsequent loop
                while (p->mnt.mnt_root != q->mnt.mnt_root)
			p = next_mnt(p, old);
could be rewritten as
		while (p->mnt.mnt_root != Origin(q)->mnt.mnt_root)
			p = next_mnt(p, old);
and in that form it's really obvious that p will not advance past
Origin(q), nevermind running out of nodes.

So on the next iteration the property still holds.  There's no way
for your added checks to trigger.

There *IS* a bug in that logics, though - mntns binding can have
a file bound on top of it.  In such case it is possible to have
p behind the Origin(q) for a (short) while.  It's not going to
cause serious problems, but that's certainly a non-obvious
behaviour and a comment needed to explain why it's not problem
is certainly longer than the one-liner change eliminating the
oddity.  Note that running into mnt_root mismatch means that p
is currently pointing to mntns binding we'd skipped when copying.
So let's skip the subtree in the same way copy_tree() did...

The bottom line:
	* your NULL pointer checks could never trigger; if you *do* have
a reproducer, please post it.
	* there's a (pretty harmless) bug in that code, but it is not
fixed by your patch.
	* see if your tool is any happier with the patch below; I would
be rather surprised if it did, but...

diff --git a/fs/namespace.c b/fs/namespace.c
index df137ba19d37..c80f422084eb 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3515,8 +3515,9 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 		q = next_mnt(q, new);
 		if (!q)
 			break;
+		// an mntns binding we'd skipped?
 		while (p->mnt.mnt_root != q->mnt.mnt_root)
-			p = next_mnt(p, old);
+			p = next_mnt(skip_mnt_tree(p), old);
 	}
 	namespace_unlock();