kern_mmap: add a variant that allows caller to inspect fp

[kevans]

Summary

Linux mmap rejects mmap() on a write-only file with EACCES. linux_mmap_common currently does a fun dance to grab the fp associated with the passed in fd, validates it, then drops the reference and calls into kern_mmap(). Doing so is perhaps both fragile and premature; there's still plenty of chance for the request to get rejected with a more appropriate error.

This change alleviates the need to do this by providing a kern_mmap variant that allows the caller to inspect the fp just before calling into the fileop layer. The callback takes flags, prot, and maxprot as one could imagine scenarios where any of these, in conjunction with the file itself, may influence a caller's decision.

The file type check in the linux compat layer has been removed; EINVAL is seemingly not an appropriate response to the file not being a vnode or device. The fileop layer will reject the operation with ENODEV if it's not supported, which more closely matches the common linux description of mmap(2) return values.

[kib]

sys/compat/linux/linux_mmap.c

143 ↗

(On Diff #66166)

This is awful.

[kevans]

sys/compat/linux/linux_mmap.c

143 ↗

(On Diff #66166)

This may be the best response I've ever received in a code review. =-)

What would you recommend instead? My initial instinct would be that the fp->f_type checks are useless and we should consider adding a kernel-only MAP_* flag that enforces (fp->f_flag & FREAD) != 0 in kern_mmap to maintain that aspect and drop this whole block.

[kib]

sys/compat/linux/linux_mmap.c

143 ↗

(On Diff #66166)

I thought about a callback provided to kern_mmap() and called with fp.

A flag might work as well, but it should be a separate kflags argument. Perhaps kflags is better then callback.

[kevans]

sys/compat/linux/linux_mmap.c

143 ↗

(On Diff #66166)

I think I'd be inclined towards the callback (taking fp and map flags), personally. It's more flexible in case we need to do some other inspection on the file, and it's hard to enumerate/envision the kinds of scenarios we may need this for since it's the linux compat layer.

I would suspect that we're more likely to churn less with a callback than if we add kflags and realize there's some other weird behavior we need to maintain. I don't think there's anything else done in kern_mmap that can't be handled prior to entry without stuff like this.

[kib]

My point what I wrote that the drop is awful is that the checks done before the drop are invalidated. Userspace might close/reopen the fd with the same number while we dropped it.

I am not sure about breakage from that race, but I assumed that the checks for the type were used for a reason.

[kevans]

In D22977#503681, @kib wrote:

My point what I wrote that the drop is awful is that the checks done before the drop are invalidated. Userspace might close/reopen the fd with the same number while we dropped it.

ah, yeah, that's the much more lethal issue.

I am not sure about breakage from that race, but I assumed that the checks for the type were used for a reason.

I did some poking around- the type check was introduced in rS166727, originally restricting mmap(2) to DTYPE_VNODE. Unfortunately the commit message doesn't really describe this part of the change, but the mmap LTP test cases don't seem to be any more broken than they were before and I suspect there's not any real code out there relying on EINVAL for "wrong file type".