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();
