vfs: stop refing freed mount points in vop_stdgetwritemount

[mjg]

Summary

The address is read without any locks held and can be stale. This alone is not a big problem since mounts are type-stable. However, changing the refcount can possibly mess with the structure after it got reassigned for another mount point (which also clears the count). Moreover the current code makes it impossible to ensure everyone is done with the mount point at least in terms of modifying it (excluding locking).

This will be of significance with other patches.

[kib]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

So what about this comment ?

[mjg]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

Caller-visible state is roughly the same in that the mount point is either NULL or what was found at the time, but it can be stale. So the comment can be reworded to be clear or removed altogether. I don't think it is that useful in the first place as the only real consumer drops the vnode interlock anyway again making the vnode suspetible to losing the mp.

[kib]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

Can it be ? Now that you both ref and check vp->v_mount under the mnt lock, I suspect that the vnode cannot be removed from the mnt queue behind us, and then the mp cannot be reused since we keep the reference.

[mjg]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

• original code refs in a very racy manner possibly having to backpedal from it - this is taken care of
• even with reference taken the vnode mount point can still be changed to NULL, therefore a refed (but stale) mount can still be returned

I think the comment was stating both of these, with the first no longer being the case.

So how about either removing this comment altogether or replacing it with +/-:

/*
 * Note that despite the reference held ->v_mount can still become NULL after drop
 * the lock because of forced unmount, which is not a problem.
 */

[kib]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

This is better. For the case 2, it was possible that mp was reused for different mount, which is not allowed after you added the lock. It can only be that the vnode is reclaimed and no longer associated with the mp, but mp cannot be reused for anything yet.

[mjg]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

yes, but that's in part what backpedalling was for

So how about this:

Note that having a reference does not prevent forced unmount from setting ->v_mount to NULL after the lock gets release.

[kib]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

It is still somewhat implicit, by describing what does occur (and which is relatively obvious), instead of describing what consequences the consumer should cope with.

[mjg]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

Ok, how about this:

Note that having a reference does not prevent forced unmount from setting ->v_mount to NULL after the lock gets released. This is of no consequence for typical consumers (most notably vn_start_write) since the in this case the vnode is VI_DOOMED. The only thing to do is to unref the returned moint point (if any).

[kib]

sys/kern/vfs_default.c

597 ↗

(On Diff #61251)

... the vnode is VI_DOOMED, and the mount point itself is at some stage of unmount.

[mjg]

Note that having a reference does not prevent forced unmount from setting ->v_mount to NULL after the lock gets released. This is of no consequence for typical consumers (most notably vn_start_write) since in this case the vnode is VI_DOOMED. Unmount might have progressed far enough that its completion is only delayed by the reference obtained here. The consumer only needs to concern itself with dropping it.