Return ENOTSUP for mmap/mprotect with prot not subset of prot_max

[emaste]

Summary

From POSIX,

[ENOTSUP]
    The implementation does not support the combination of accesses
    requested in the prot argument.

This fits the case that prot contains permissions which are not a subset of prot_max.

[brooks]

This makes sense to me. I think I might have missed this due to a formatting bug in the posix manpage.

lib/libc/sys/mmap.2

434 ↗

(On Diff #68845)

Does it make sense to sort by error value?

[emaste]

lib/libc/sys/mmap.2

434 ↗

(On Diff #68845)

Probably, although I edited mprotect.2 first and there EACCESS is at the end.

POSIX sorts alphabetically so I can sort mprotect.2 first and then put these in the right spot.

[jrtc27]

Hm, just stumbling across this now and it feels wrong; the combination _is_ supported, it's just not permitted for these pages. Would EACCES not make more sense?

[EACCES]
    The prot argument specifies a protection that violates the access permission the process has to the underlying memory object.

PROT_MAX effectively sets the access permission the process has to the underlying memory. Things get a bit weird when you have both an RX and an RW mapping to the same object, but that's a bit of an edge case, and I feel it's still the error code that would cause least surprise. Especially given how POSIX describes them in general:

[EACCES]
    Permission denied.
[ENOTSUP]
    Not supported (may be the same value as [EOPNOTSUPP]).
[EOPNOTSUPP]
    Operation not supported on socket (may be the same value as [ENOTSUP]).

"Permission denied" feels most appropriate as a message to me. Is there a reason not explained here why that wasn't chosen?

[emaste]

I think your argument is reasonable and I doubt it would cause any trouble to change it - @brooks do you agree?

[brooks]

This isn't about permissions on the existing page. This is: "the programmer said I want RX, but never more than R". EDOOFUS might be more appropriate, but is non-POSIX.

[jrtc27]

In D23843#606426, @brooks wrote:

This isn't about permissions on the existing page. This is: "the programmer said I want RX, but never more than R". EDOOFUS might be more appropriate, but is non-POSIX.

The two calls may not have been written by the same programmer if one or both are in libraries.

[cem]

I agree that something else may make more sense. I'd suggest EPERM (as opposed to EACCES, which explicitly mentions file access permissions) or EINVAL (generic bogus argument) as possibilities. I don't oppose EACCES either. I think EDOOFUS is pretty much always wrong.

1 EPERM Operation not permitted. An attempt was made to perform an
        operation limited to processes with appropriate privileges or to
        the owner of a file or other resources.

13 EACCES Permission denied. An attempt was made to access a file in a
        way forbidden by its file access permissions.

[brooks]

In D23843#606427, @jrtc27 wrote:

In D23843#606426, @brooks wrote:

This isn't about permissions on the existing page. This is: "the programmer said I want RX, but never more than R". EDOOFUS might be more appropriate, but is non-POSIX.

The two calls may not have been written by the same programmer if one or both are in libraries.

This isn't about a previous call. In the mprotect case we haven't even verified that there is a mapping there (beyond the 32-bit overflow check). It's just checking that any requested max_prot is a superset of the requested prot. I agree that if the vm_map_protect call fails due to the max_prot on the mapping then ENOTSUP isn't a good error, but that's not what this review is about.

[cem]

Hm, that kind of check is usually EINVAL.

[jrtc27]

In D23843#606431, @brooks wrote:

In D23843#606427, @jrtc27 wrote:

In D23843#606426, @brooks wrote:

This isn't about permissions on the existing page. This is: "the programmer said I want RX, but never more than R". EDOOFUS might be more appropriate, but is non-POSIX.

The two calls may not have been written by the same programmer if one or both are in libraries.

This isn't about a previous call. In the mprotect case we haven't even verified that there is a mapping there (beyond the 32-bit overflow check). It's just checking that any requested max_prot is a superset of the requested prot. I agree that if the vm_map_protect call fails due to the max_prot on the mapping then ENOTSUP isn't a good error, but that's not what this review is about.

Oh, then EINVAL makes much more sense. mmap(2) currently has:

[EINVAL]		An invalid value was passed in the prot	argument.

Is this not just a specific case of that?