Summary: Apply fixes uncovered in vfs_bio.c via static analysis.

[darrick.freebsd_gmail.com]

Test Plan

Verified kernel build/boot.

[cem]

sys/kern/vfs_bio.c

2565–2566 ↗

(On Diff #40058)

Is it actually valid for b_vp to be NULL here? I suspect Coverity may not understand the buffer lifecycle fully. If it is valid, can we come up with a testcase that exercises it?

Assuming it's valid, I think it might make sense to pull this (sub)conditional out into a subroutine at this point. This if-statement is huge and unwieldy already.

bool
vn_is_vfcfc_network(const struct vnode *vp)
{
    return (vp != NULL && vp->v_mount != NULL &&
       vp->v_mount->mnt_vfc->vfc_flags & VFCF_NETWORK != 0);
}

[darrick.freebsd_gmail.com]

sys/kern/vfs_bio.c

2565–2566 ↗

(On Diff #40058)

I'm not 100% sure whether it's possible. I'd assume it doesn't happen often. If it happened with any degree of frequency then the kernel would panic.

[darrick.freebsd_gmail.com]

sys/kern/vfs_bio.c

2565–2566 ↗

(On Diff #40058)

The path that explicitly releases bp->b_vp and sets it to NULL (call to be brelvp) is only called when B_VMIO is not set. The check that could potentially derefer that NULL bp->b_vp pointer is only reached when B_VMIO is set. It may be possible for bp->b_vp to be NULL when brelse() is called, but I'm not sure what those circumstances would be.

[kib]

sys/kern/vfs_bio.c

2565–2566 ↗

(On Diff #40058)

Right, VMIO buffers cannot exist with b_vp not assigned, they must always belong to the correct vnode. I prefer the buggy code manifests itself with the NULL deref and panic.

Nonetheless, there is one thing to improve in the condition. Note that the bp->b_vp->v_mount is dereferenced twice. First it is done to check for non-NULL, and second time to dereference more and test the flag. Theoretically, locked buffer must prevent the vnode reclamation and v_mount going from non-NULL to NULL, but practically I think that this might be violated sometimes, esp. for devfs.

I suggest to create a local struct mount * variable where to fetch the v_mount value once and then use it for check.

4980 ↗

(On Diff #40058)

VOP_GETPAGES must be called with at least one page invalid, and since we downgrade busy state of the page from exclusive to shared without race, bread_gb() must be called at least once. This would initialize error.

But I do not object against this formal change.

[darrick.freebsd_gmail.com]

sys/kern/vfs_bio.c

2565–2566 ↗

(On Diff #40058)

I can create the local struct mount *.

Should we assert b_vp != NULL or not B_VMIO?

[kib]

sys/kern/vfs_bio.c

2565–2566 ↗

(On Diff #40058)

b_vp != NULL is checked by hardware, in robust manner.
I do not see a need to add asserts.

[darrick.freebsd_gmail.com]

Incorporate CR feedback.

[kib]

sys/kern/vfs_bio.c

2469 ↗

(On Diff #40236)

There is no need to calculate v_mount that early. In fact it might be best to do (v_mnt = (bp->b_vp->v_mount) != NULL inside the if ().

Also, I would suggest to take the opportunity and revert the conditions together with the binary operations inside the statement. I.e. remove the '!' before the () and pass it down, replacing && by ||.

[darrick.freebsd_gmail.com]

sys/kern/vfs_bio.c

2469 ↗

(On Diff #40236)

That if-statement is already pretty complex, do we want to introduce an assignment side-effect to it? I will shift the !() as you suggest.

[kib]

sys/kern/vfs_bio.c

2469 ↗

(On Diff #40236)

Do as you prefer, my main point was that the calculation of the v_mnt is too early, there are returns where the value is simply unused.

[darrick.freebsd_gmail.com]

• Incorporate CR feedback.