Bug 253500 - [fusefs]: F_SETLKW is treated as F_SETLK and F_UNLCK is ignored
Summary: [fusefs]: F_SETLKW is treated as F_SETLK and F_UNLCK is ignored
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.2-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-14 11:25 UTC by John Millikin
Modified: 2021-04-08 21:56 UTC (History)
3 users (show)

See Also:
asomers: mfc-stable13+
asomers: mfc-stable12+
asomers: mfc-stable11-


Attachments
quick-n-dirty patch to fix fcntl locking in FUSE (1.18 KB, patch)
2021-02-14 11:25 UTC, John Millikin
no flags Details | Diff
Fixes two bugs in fuse_vnop_advlock (5.27 KB, patch)
2021-02-27 19:45 UTC, Alan Somers
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Millikin 2021-02-14 11:25:33 UTC
Created attachment 222425 [details]
quick-n-dirty patch to fix fcntl locking in FUSE

There's at least two bugs in fcntl lock handling that get tickled by FUSE:

  1. Calls to `fcntl(fd, F_SETLKW, &lock)` are showing up in the FUSE server as
     a FUSE_SETLK (should be FUSE_SETLKW).

  2. fcntl calls to _clear_ a lock, by using F_SETLK with `l_type = F_UNLCK`,
     never get passed to userspace at all.

It looks like both incorrect behaviors are caused by `kern_fcntl()`:

  * F_SETLKW is handled by a switch-case fallthrough to the F_SETLK path, but the
    `VOP_ADVLOCK()' call is hardcoded to use `F_SETLK'.

  * In the nested switch over the lock type, several calls to `VOP_ADVLOCK()' are
    performed using the lock type (e.g. `F_UNLCK') where they should use the fcntl
    opcode. This causes `fuse_vnop_advlock()' to return EINVAL.

Attached is a patch that fixes both issues according to my test suite. I'm not very familiar with the FreeBSD kernel, so note that this patch might not do the right thing for other filesystem drivers.
Comment 1 Konstantin Belousov freebsd_committer 2021-02-14 11:33:40 UTC
The patch is wrong, of course.  It breaks calling conventions for lf_advlockasync()
which is the implementation for all advlock vops (except fuse and partially nfs).

You need to adopt fuse to the interface provided by VOP.  If there is missed
information at the VOP call time, describe what sort of info is lost.
Comment 2 Alan Somers freebsd_committer 2021-02-24 16:26:23 UTC
Kib's right.  The VOP_ADVLOCK interface isn't exactly the same as fcntl.  It's not legal for op to be F_SETLKW.  Instead, it should be F_SETLK and the flags field should contain F_WAIT.  You can fix this in fuse_vnop_advlock .  The second problem is also likely caused by fuse_vnop_advlock, but I don't have time to debug it right now.

If you're planning to submit a fix, could you please also fix and/or extend the tests in tests/sys/fs/fusefs/locks.cc ?
Comment 3 Alan Somers freebsd_committer 2021-02-27 19:45:43 UTC
Created attachment 222867 [details]
Fixes two bugs in fuse_vnop_advlock

John, can you please test the attached patch?
Comment 4 commit-hook freebsd_committer 2021-03-18 23:10:50 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=929acdb19acb67cc0e6ee5439df98e28a84d4772

commit 929acdb19acb67cc0e6ee5439df98e28a84d4772
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-03-18 20:27:27 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2021-03-18 23:09:10 +0000

    fusefs: fix two bugs regarding fcntl file locks

    1) F_SETLKW (blocking) operations would be sent to the FUSE server as
       F_SETLK (non-blocking).

    2) Release operations, F_SETLK with lk_type = F_UNLCK, would simply
       return EINVAL.

    PR:             253500
    Reported by:    John Millikin <jmillikin@gmail.com>
    MFC after:      2 weeks

 sys/fs/fuse/fuse_vnops.c       | 10 +++++++---
 tests/sys/fs/fusefs/flush.cc   | 12 ++++++++++-
 tests/sys/fs/fusefs/locks.cc   | 45 +++++++++++++++++++++++++++++++++++++++++-
 tests/sys/fs/fusefs/release.cc | 12 ++++++++++-
 4 files changed, 73 insertions(+), 6 deletions(-)
Comment 5 commit-hook freebsd_committer 2021-04-08 02:17:54 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=32be2332d10f6423def9d4c41d7e7707a97abae6

commit 32be2332d10f6423def9d4c41d7e7707a97abae6
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-03-18 20:27:27 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2021-04-08 02:15:42 +0000

    fusefs: fix two bugs regarding fcntl file locks

    1) F_SETLKW (blocking) operations would be sent to the FUSE server as
       F_SETLK (non-blocking).

    2) Release operations, F_SETLK with lk_type = F_UNLCK, would simply
       return EINVAL.

    PR:             253500
    Reported by:    John Millikin <jmillikin@gmail.com>

    (cherry picked from commit 929acdb19acb67cc0e6ee5439df98e28a84d4772)

 sys/fs/fuse/fuse_vnops.c       | 10 +++++++---
 tests/sys/fs/fusefs/flush.cc   | 12 ++++++++++-
 tests/sys/fs/fusefs/locks.cc   | 45 +++++++++++++++++++++++++++++++++++++++++-
 tests/sys/fs/fusefs/release.cc | 12 ++++++++++-
 4 files changed, 73 insertions(+), 6 deletions(-)
Comment 6 commit-hook freebsd_committer 2021-04-08 21:35:48 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9a9c9e744b51e00f3446afb922500e40845cddfa

commit 9a9c9e744b51e00f3446afb922500e40845cddfa
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-03-18 20:27:27 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2021-04-08 21:33:55 +0000

    fusefs: fix two bugs regarding fcntl file locks

    1) F_SETLKW (blocking) operations would be sent to the FUSE server as
       F_SETLK (non-blocking).

    2) Release operations, F_SETLK with lk_type = F_UNLCK, would simply
       return EINVAL.

    PR:             253500
    Reported by:    John Millikin <jmillikin@gmail.com>

    (cherry picked from commit 929acdb19acb67cc0e6ee5439df98e28a84d4772)

    fusefs: fix a dead store in fuse_vnop_advlock

    kevans actually caught this in the original review and I fixed it, but
    then I committed an older copy of the branch.  Whoops.

    Reported by:    kevans
    Differential Revision:  https://reviews.freebsd.org/D29031

    (cherry picked from commit 9c5aac8f2e84ca4bbdf82514302c08c0453ec59b)

 sys/fs/fuse/fuse_vnops.c       |  9 ++++++---
 tests/sys/fs/fusefs/flush.cc   | 12 ++++++++++-
 tests/sys/fs/fusefs/locks.cc   | 45 +++++++++++++++++++++++++++++++++++++++++-
 tests/sys/fs/fusefs/release.cc | 12 ++++++++++-
 4 files changed, 72 insertions(+), 6 deletions(-)