Fix a vnode locking bug in fuse_vnop_advlock.
ClosedPublic
Actions

Authored by asomers on Jan 2 2021, 11:13 PM.

Details

Reviewers

Commits

rGc76461e064b6: Fix a vnode locking bug in fuse_vnop_advlock.
rG542711e52079: Fix a vnode locking bug in fuse_vnop_advlock.

Summary

Fix a vnode locking bug in fuse_vnop_advlock.

It was introduced by subversion r346170.

MFC after: 2 weeks

Test Plan

Existing test suite with DEBUG_VFS_LOCKS

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 35901
Build 32790: arc lint + arc unit

Event Timeline

asomers created this revision.Jan 2 2021, 11:13 PM

Herald added a subscriber: imp. · View Herald TranscriptJan 2 2021, 11:13 PM

asomers requested review of this revision.Jan 2 2021, 11:13 PM

Harbormaster completed remote builds in B35901: Diff 81561.Jan 2 2021, 11:13 PM

Ok, so the bug was that we never took any lock at all? But we're also moving a block of code surrounding the new vn_lock(); operation, which isn't related to our not-locking-the-vp-bug. It looks like we want to reject EINVAL flags before we start locking, which is fine. I would just like to see a better commit message than "fix a [non-specific] bug." Something like "add missing lock in fuse advlock. also, reject invalid parameters sooner (style)."

sys/fs/fuse/fuse_vnops.c
429–433	Is assuming `v_mount` is non-NULL and of type `struct fuse_data *` with `vp` unlocked safe?

This revision is now accepted and ready to land.Jan 3 2021, 12:58 PM

asomers added inline comments.Jan 3 2021, 4:00 PM

sys/fs/fuse/fuse_vnops.c
429–433	Good question. I don't know what protects a vnode's v_mount field. But I see that it's checked without a vnode lock elsewhere, in ext2_pathconf, nfs_advlockasync, nfs_advise, and vn_generic_copy_file_range. So I'm going to assume that it's ok.