You are mixing styles. I usually test the flags explicitly with != 0.

In this case it is because the code comes from different functions whcih used different styles. :-/

I actually prefer to not use the != 0 for testing single flags, but I will update these. I think vn_mmap() likely has a mix of styles as well now for the same reason.

I don't know why this check is here and why it returns 0 instead of EINVAL.

Humm, it goes all the way back to when vm_mmap.c was first imported into BSD in 1990. :-/

I don't think we pass a size of zero explicitly to vm_mmap(). Hmm, I thought sys_mmap() was filtering all these out, but it is not. Oddly enough, if you do something like this in an old binary:

p = mmap(NULL, 0, PROT_READ, MAP_SHARED, fd, PAGE_SIZE / 2);

You will get back a valid pointer for the first page of the file. If you pass a position of 0 you would get back some address that might have been mapped (but for something else) or might not have been.

I would be inclined to move the size == 0 handling up into sys_mmap() itself. Perhaps bail early with a 0 return value for uap->len == 0. However, that breaks the weird case above. Do we know if any binaries are doing zero-length mmaps with non-page-aligned file offsets and expecting to get a page? I guess I could always do the size == 0 check after the pageoff adjustment to size. I would then want vm_mmap() to fail with EINVAL because I don't think in-kernel drivers should be doing zero-length mappings.

I don't know if it would be better to perform the EINVAL checks later in this function before doing the racct stuff. There doesn't seem to be anything to unwind these racct reservations if the mapping fails.

Hmm, I guess if they fail later on it is sort of harmless since the next mapping attempt will "fix" the accounting.

What prevents the vnode reclaim and corresponding vp->v_mount from invalidation between two checks in the if () test ?

You call vm_object_deallocate() both in the fo_mmap implementations, e.g. it is done both in vn_mmap and shm_mmap, and in the error path of the vm_mmap_object(). Is it intended, or the deallocation happens one more time than it should ?

Also, why do you need to pass writecounted down to vm_mmap_object ? You get the control back in vn_mmap, so you could release write count there,

Mmm, is the ref count from the fp enough (fp is held by the caller (sys_mmap) for the duration of this function)? This is the same now as in the old code. I am also assuming that this reference from the fp is enough to make v_rdev stable for the call to vm_mmap_cdev() in vn_mmap().

Ah, that is true. I think I will remove it from vm_mmap_object() (I had fixed vm_mmap() to drop it on error as well). I think it is cleaner for the caller (e.g. fo_mmap methods) to drop the reference explicitly on error since they gained the reference.

Unfortunately I cannot move the writecounted hack since vm_mmap() with OBJT_VNODE needs to be able to do it as well. Well, I guess I could duplicate it in both vm_mmap() and vn_mmap(). That might actually be cleaner since it is a kind of cleanup only for vm_mmap_vnode() and would be done at the same level as the call to vm_mmap_vnode().

Which do you prefer? Doing the deallocates and writecounted checks in the callers will result in more duplication. The reason I can see in favor of doing it is that it makes the vm_mmap_object() API a bit simpler and keeps code changes at the same level (e.g. writecounted is only in functions that directly use vm_mmap_vnode()).

Actually, I've broken the writecounted updates now in that if the racct errors trigger then they won't get backed out. Moving the cleanup into the callers will fix this so I will do that.

A struct mount is type stable; the vp->v_mount could only transition from valid pointer to NULL. Usual solution is to ensure that vp->v_mount is evaluated only once, i.e. cache it into local variable. Look for instance at the MNT_SHARED_WRITES() or MNT_EXTENDED_SHARED() inlines in the sys/mount.h.

I agree with the move of dereference to the callers.

I am not aware of such cases, but I would also probably be extra-cautious. Consumers of the mmap interface tend to exercise all corner cases, usually.

I kept the MNT_NOEXEC check to err on the side of caution though I know Konstantin stated on IRC he feels it isn't very useful.

I was originally worried that having this separate from vn_mmap would result in a lot of duplication, but I don't think it does. One thing I am assuming is that hwpmc doesn't care about executable mappings belonging to devices.

In theory we could actually dispense with devfs_fp_check() and just pass fp->f_data down as the cdev pointer to vm_mmap_cdev(). However, I think it is cleaner to be more consistent. This also results in td_fpop being handled the way it is for other operations (and sys_mmap() no longer has to mess with it).

Note that vm_mmap_cdev() also calls dev_refthread() internally. Although seemingly innocent, I do not like the recursion for si_threadcount.

Might be, vm_mmap_cdev() should be split into two functions, one which does refthread wrapping and calls another. Then another could be used there as well. Also, it should reduce the number of places where dev_relthread() is called in vm_mmap_cdev(), making the logic cleaner.

if (error != 0)

For modern binaries, the function returns an error earlier.

Might be, we should finally add CTASSERTs() for this repeated claim.

Is this check and cleanup still needed in vm_mmap_vnode() ? Unless I am confused, there are two callers of vm_mmap_vnode(), and both of them check writecounted. vn_mmap() would need to do
error = vm_mmap_vnode(); if (error == 0) error = vm_mmap_object(); if (error != 0) { if (writecounted) ...}
to not skip writecounted cleanup, but this should be fine.

Aren't this removes support for mmapping shm objects using vm_mmap() ? Do we need the feature ?

I can push the refthread into vm_mmap() around vm_mmap_cdev().

I suspect that all the callers of vm_mmap() with OBJT_DEVICE already hold a thread reference on the cdev, but there doesn't appear to be way to assert that easily.

Ah, yes, the function returns the error, not the binary itself.

The one other bit of confusion is that we aren't consistent with prot. vm_mmap() and fo_mmap() generally accept the mmap arg (PROT_*) not a vm_prot_t, but it is passed as a vm_prot_t. It might be more accurate for prot to be
an int that is always the mmap args and have vm_mmap_object be the only place doing the implicit conversion.

Actually, I think it might be better if I fix vm_mmap_cdev/vnode and fo_mmap to assume VM_PROT_* instead.

On error vm_mmap_vnode() doesn't return a valid object pointer (which is needed to pass to vnode_pager_update_writecount()).

Hmm, there appears to be an old bug here in that if vm_pager_allocate() fails and we had set writecounted previously, then we will pass a NULL object to vnode_pager_update_writecount.

We should not. I only added that for the system call to map them. I don't know of any drivers that are mapping them in userland address spaces via ioctls.

The only place that calls vm_mmap() with OBJT_DEVICE is actually drm_mapbufs(). Those are always done from an ioctl, and so devfs_ioctl_f() already holds a reference. OTOH, I'd have to get to the devsw somehow. Probably clearer to leave it as-is.

There is a ')' missing.