Page MenuHomeFreeBSD

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

Authored by darrick.freebsd_gmail.com on Mar 7 2018, 11:50 PM.

Details

Test Plan

Verified kernel build/boot.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem added inline comments.Mar 8 2018, 12:41 AM
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);
}
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.

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 added inline comments.Mar 8 2018, 10:25 AM
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.

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 added inline comments.Mar 8 2018, 8:54 PM
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.

Incorporate CR feedback.

kib added inline comments.Mar 13 2018, 10:14 AM
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 ||.

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 added inline comments.Mar 13 2018, 5:49 PM
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.

  • Incorporate CR feedback.
kib accepted this revision.Mar 13 2018, 10:41 PM
This revision is now accepted and ready to land.Mar 13 2018, 10:41 PM
cem accepted this revision.Mar 14 2018, 9:58 PM
This revision was automatically updated to reflect the committed changes.