fhopen: Enable handling of O_PATH, fix some bugs

[rmacklem]

sys/kern/vfs_syscalls.c

4686

This looks fine, although I'll admit I would
have written the clause as "(fmode & (O_CREAT | O_PATH)) != 0
(To me, this somehow makes what it is doing more obvious?)

But go whichever way you prefer.

[kib]

You completely disallowed O_PATH for fhopen(), but why? I would instead make it behave same as normal open() by ignoring mode for O_PATH.

[rmacklem]

Without this patch:
kern_fhopen() requires FREAD or FWRITE.
It checks for these and fails with EINVAL if neither are set.

On the other hand, a fhopen(&fh, O_RDONLY | O_NAMEDATTR | O_PATH)
does work and openat() can use the fd.
So for fhopen(), O_RDONLY is not ignored and is required.
(A comment says that any mode other than O_EXEC is ignored for O_PATH.)

It seems that either O_PATH should fail as the above patch does
or
O_EXEC should be allowed for fhopen()

I don't know which is preferable, although I will note that a NFS server
does not handle "exec" separately from "read" and I thought the idea
behind fhopen() was a userspace NFS server?

[rmacklem]

Just to clarify what the previous comment was talking about.
Without this patch:
fhopen(&fh, O_RDONLY | O_NAMEDATTR | O_PATH) works
but
fhopen(&fh, O_EXEC | O_NAMEDATTR | O_PATH) fails with EINVAL
(You can basically ignore the O_NAMEDATTR. I just did the tests with
some test code I had lying about for O_NAMEDATTR.)

[markj]

It seems that either O_PATH should fail as the above patch does or O_EXEC should be allowed for fhopen()

I think that fhopen(&fh, O_EXEC | O_RDWR) would be permitted, but this is a bug. Note that O_RDONLY == 0x0.

In D49717#1133727, @kib wrote:

You completely disallowed O_PATH for fhopen(), but why? I would instead make it behave same as normal open() by ignoring mode for O_PATH.

Mostly because of the pre-existing check for FREAD|FWRITE. If the caller is always expected to do I/O to the file, then O_PATH doesn't make much sense.

As Rick notes, use of O_EXEC is de facto disallowed as well. I suppose we could unify the flag handling with openatfp() and permit both O_EXEC and O_PATH.

[kib]

In D49717#1133727, @kib wrote:

You completely disallowed O_PATH for fhopen(), but why? I would instead make it behave same as normal open() by ignoring mode for O_PATH.

Mostly because of the pre-existing check for FREAD|FWRITE. If the caller is always expected to do I/O to the file, then O_PATH doesn't make much sense.

As Rick notes, use of O_EXEC is de facto disallowed as well. I suppose we could unify the flag handling with openatfp() and permit both O_EXEC and O_PATH.

I think this would be the best. I do not think that the argument that before, read|write was required, blocks addiing O_PATH support.

[markj]

Unify handling of access modes and O_PATH between openat() and fhopen().

[kib]

sys/kern/vfs_syscalls.c

1165

4710

I think this statement can be written as

if (named_attr ^^ (flags & O_NAMEDATTR) != 0)

but up to you

[kib]

sys/kern/vfs_syscalls.c

1167

BTW, I think I would use a different function signature:

error = openflags(&flags);

i.e.

int openflags (int *flagsp);

It is more in-line with the common kernel pattern to return the error instead of a value.

[markj]

Apply kib's suggestion; add a comment.

[kib]

Thanks

[markj]

Fix incomplete O_PATH support: we were failing to set f_ops =
path_fileops. Deduplicate some more code between open() and fhopen().

This also fixes another bug in fhopen(): it would unconditionally set
the file ops to vnops, but this is wrong for FIFOs and for some devfs
nodes.