Fix autofs triggering problem.

[trasz]

Summary

Assume you have an NFS server, 192.168.1.1, with share "share". The
patch below fixes a problem where "mkdir /net/192.168.1.1/share/meh"
would return spurious error instead of creating the directory.

The scenario is kind of complicated to explain, but it all boils
down to calling VOP_LOOKUP() for the target filesystem (NFS) with
wrong dvp - autofs vnode instead of the filesystem root mounted
over it.

[kib]

sys/fs/autofs/autofs_vnops.c

270 ↗

(On Diff #13727)

I agree, in principle, with this change.

sys/kern/vfs_lookup.c

787 ↗

(On Diff #13727)

This cannot be right. If this change is nop, your subtraction from cn_consume above is sometimes wrong. Add KASSERT() to autofs_lookup() to ensure that you never subtract more than allowed, and it should catch the problematic case.

[trasz]

Take two. This removes the negative offsets, but preserves the simplicity
of the mechanism. I've spent some time on trying to figure out how to
"roll back" the state externally, and it seems way more complicated than
this. Also, this way we keep namei internals... well, internal.

[kib]

I think this is almost fine.

Do not use the flag. Instead, do the same as was done with EJUSTRETURN. Usurp a kernel-only error, e.g. ENOIOCTL, and return it from vop_lookup() implementation to indicate the request to relookup. Of course, existing ENOIOCTL usage must be audited to ensure that no conflict arise.

I believe that this would also allow you to avoid unneccesary vref/vget on dvp.

sys/kern/vfs_lookup.c

865 ↗

(On Diff #13846)

Why couldn't this be goto unionlookup ?

[trasz]

The problem with doing it in the error path is that we still need the code for crossing the mounts which happens slightly later (the code path with call to VFS_ROOT()). Also, I think it would complicate the interface: now it's all about setting a single flag; all the rest is the same as with other lookups. Perhaps I could make the code path shorter, but it would complicate the "big picture", and it seems kind of pointless anyway: this operation is in no way important to performance; when it happens, there is also the userland daemon involved, and either a network operation (NFS) or local disk access (-media autofs map).

[kib]

In D5442#117730, @trasz wrote:

The problem with doing it in the error path is that we still need the code for crossing the mounts which happens slightly later (the code path with call to VFS_ROOT()). Also, I think it would complicate the interface: now it's all about setting a single flag; all the rest is the same as with other lookups. Perhaps I could make the code path shorter, but it would complicate the "big picture", and it seems kind of pointless anyway: this operation is in no way important to performance; when it happens, there is also the userland daemon involved, and either a network operation (NFS) or local disk access (-media autofs map).

I looked over the overall structure of lookup() again, and I object against doing what you want, with the flag. The direction of the flow of the flag-passed information is clear, and your flag contradicts it.

If you want to keep your current structure of hack, check the specific error code right after the VOP_LOOKUP() call and set local boolean for later use, while reset error to 0 immediately.

[trasz]

Don't use cn_flags to pass information from the filesystem to lookup();
use custom error code instead.

[kib]

sys/fs/autofs/autofs_vnops.c

269 ↗

(On Diff #14017)

Do not utilize userspace error codes, select one from the negative (kernel) set.

sys/kern/vfs_lookup.c

734 ↗

(On Diff #14017)

Do not do the strcmp. Error returned should be the only test to perform.

[trasz]

Take three: use kernel-only error code (add a new instead of reusing existing
ones to make the code more readable and to make it easier to grep for it),
follow the VOP_LOOKUP() interface better by using NULL vpp, as is done in other
cases of non-zero return, add an option to switch back to the old way (just in
case), and prevent nullfs and unionfs users from foot-shooting.

[kib]

sys/fs/autofs/autofs_vnops.c

271 ↗

(On Diff #14189)

This is wrong. Read about LK_EXCLOTHER.

sys/fs/nullfs/null_vnops.c

433 ↗

(On Diff #14189)

Why is this needed ?

sys/kern/vfs_lookup.c

736 ↗

(On Diff #14189)

Please put the new block after the unionlookup detection, i.e. move it after the next (in your patch) if().

787 ↗

(On Diff #14189)

You must restore cn_lkflags for your case as well.

797 ↗

(On Diff #14189)

Why don't you reset cn_cn_consume to zero for relookup ?

[trasz]

Take 4 - get rid of nullfs/unionfs changes (they make the failure mode
slighly more user-friendly, but it's not critical, and I don't quite
understand the failure mode); get rid of fallback to old code, which
turned out to be _very_ broken.