vfs: Don't clobber namei flags in vn_open_cred()

[markj]

Summary

Otherwise NAMEILOOKUP is cleared. More generally it seems quite
surprising that the flags set by vn_open_cred() callers are not
automatically preserved. I see at least one bug due to this:
zfs_getextattr_dir()'s use of NOFOLLOW is ignored, as it doesn't also
specify O_NOFOLLOW.

After going through callers, I believe it's safe to add the flags this
way.

Fixes: 7587f6d4840f ("namei: Make stackable filesystems check harder for jail roots")

[markj]

Fix up handling of O_NOFOLLOW.

[kib]

sys/kern/vfs_vnops.c

190

I suggest changing the open2nameif() signature to

static void open2nameif(int fmode, u_int vn_open_flags, struct nameidata *ndp);

and having the 'or' hidden in the function.

[olce]

Browsing some old notes, I had noticed that cn_flags were sometimes crushed, and concluded that VN_* flags probably were just workarounds for that problem, except for VN_OPEN_INVFS.

Now, not crushing some flags on O_CREAT might introduce subtle semantics change. An audit of all direct vn_open_cred() callers does not show any problem (but make me notice a bug that the change here will fix as a side-effect, see below). However, I have not audited callers of vn_open().

The bug you noticed in zfs_setextattr_dir() about NOFOLLOW being ignored is in fact only a conceptual one, as NOFOLLOW is in fact 0. However, it seems there is a real, similar bug in openat() but with FOLLOW.

sys/kern/vfs_vnops.c

190

I'm OK with open2nameif() doing the OR (and actually more, see the other inline comment), but not really with passing ndp, which obfuscates at calling points that this function is only meant to compute cn_flags.

I'd rather suggest passing a pointer to cn_flags instead of ndp.

358–360

I would put the O_FOLLOW handling into open2nameif().

[markj]

Fix FOLLOW/NOFOLLOW handling.

[markj]

In D50531#1153900, @olce wrote:

Browsing some old notes, I had noticed that cn_flags were sometimes crushed, and concluded that VN_* flags probably were just workarounds for that problem, except for VN_OPEN_INVFS.

Now, not crushing some flags on O_CREAT might introduce subtle semantics change. An audit of all direct vn_open_cred() callers does not show any problem (but make me notice a bug that the change here will fix as a side-effect, see below). However, I have not audited callers of vn_open().

The bug you noticed in zfs_setextattr_dir() about NOFOLLOW being ignored is in fact only a conceptual one, as NOFOLLOW is in fact 0. However, it seems there is a real, similar bug in openat() but with FOLLOW.

openatfp() ORs FOLLOW into the cnflags unless O_NOFOLLOW is set, and zfs_setextattr_dir() does not specify O_NOFOLLOW. So its use of NOFOLLOW is silently getting converted to FOLLOW. Maybe it's not a real problem in practice, but it doesn't matter that NOFOLLOW is 0.

sys/kern/vfs_vnops.c

190

I did it a bit differently than both suggestions, such that open2nameif() continues to be a pure function.

[kib]

sys/kern/vfs_vnops.c

279

IMO this is too convoluted. open2namei() already set FOLLOW in !O_NOFOLLOW. So to keep the existing behavior, isn't it enough to clear FOLLOW if O_EXCL is set, without two lines above that set FOLLOW?

[markj]

• Simplify O_EXCL handling.
• Preserve NOFOLLOW from callers, inadvertently lost in the previous revision.

Now, a caller of vn_open_cred() has to explicitly specify FOLLOW, else it gets NOFOLLOW
semantics. O_FOLLOW overrides FOLLOW.

[olce]

In D50531#1153938, @markj wrote:

openatfp() ORs FOLLOW into the cnflags unless O_NOFOLLOW is set, and zfs_setextattr_dir() does not specify O_NOFOLLOW. So its use of NOFOLLOW is silently getting converted to FOLLOW. Maybe it's not a real problem in practice, but it doesn't matter that NOFOLLOW is 0.

You probably meant vn_open_cred() here. Yes, I forgot about the special handling of O_NOFOLLOW after calling open2nameif(), with indeed causes a real problem in zfs_setextattr_dir().

In D50531#1154012, @markj wrote:

O_FOLLOW overrides FOLLOW.

O_NOFOLLOW, yes. Doing so seems quite reasonable.