On Thu, Apr 01, 2021 at 07:59:19PM +0200, Christian Brauner wrote:
> I _think_ I see what the issue is. It seems that an assumption made in
> this commit might be wrong and we're missing a mnt_add_count() bump that
> we would otherwise have gotten if we've moved the failure handling into
> the unlazy helpers themselves.
>
> Al, does that sound plausible?
mnt_add_count() on _what_? Failure in legitimize_links() ends up with
nd->path.mnt zeroed, in both callers. So which vfsmount would be
affected?
Rules:
in RCU mode: no mounts pinned
out of RCU mode: nd->path.mnt and all nd->stack[i].link.mnt for
i below nd->depth are either NULL or pinned
Transition from RCU to non-RCU mode happens in try_to_unlazy() and
try_to_unlazy_next().
References (if any) are dropped by eventual terminate_walk() (after
that the contents of nameidata is junk).
__legitimize_mnt() is the primitive for pinning. Return values:
0 -- successfully pinned (or given NULL as an argument)
1 -- failed, refcount not affected
-1 -- failed, refcount bumped.
It stays in RCU mode in all cases.
One user is __legitimize_path(); it also stays in RCU mode. If it
fails to legitimize path->mnt, it will zero it *IF* __legitimize_mnt()
reports that refcount hadn't been taken. In all other cases,
path->mnt is pinned. IOW, the caller is responsible for path_put()
regardless of the outcome.
Another user is legitimize_mnt(). _That_ will make sure that
refcount is unaffected in case of failure (IOW, if __legitimize_mnt()
reports failure with refcount bumped, we drop out of RCU mode,
do mntput() and go back).
On failure in legitimize_links() we either leave nd->depth equal to zero
(in which case all nd->stack[...].link.mnt are to be ignored) or
we set it one higher than the last attempted legitimize_path() in there.
In the latter case, all entries in nd->stack below the value we put into
nd->depth had legitimize_path() called (and thus have ->mnt either NULL
or pinned) and everything starting from nd->depth is to be ignored.
nd->path handling:
1) Callers of legitimize_links() are responsible for zeroing nd->path.mnt
on legitimize_links() failure. Both do that, AFAICS.
2) in try_to_unlazy() we proceed to call legitimize_path() on nd->path.
Once that call is done, we have nd->path.mnt pinned or NULL, so nothing
further is needed with it.
3) in try_to_unlazy_next() we use legitimize_mnt() instead. Failure
of that is handled by zeroing nd->path.mnt; success means that nd->path.mnt
is pinned and should be left alone.
We could use __legitimize_mnt() in try_to_unlazy_next() (basically,
substitute the body of legitimize_mnt() there and massage it a bit),
but that ends up being harder to follow:
res = __legitimize_mnt(nd->path.mnt, nd->m_seq);
if (unlikely(res)) {
if (res < 0) // pinned, leave it there
goto out1;
else // not pinned, zero it
goto out2;
}
instead of
if (unlikely(!legitimize_mnt(nd->path.mnt, nd->m_seq)))
goto out2;
we have now.
