Bug 265736 - F_SETFL will fail yet still set O_NONBLOCK for character devices that don't implement d_ioctl
Summary: F_SETFL will fail yet still set O_NONBLOCK for character devices that don't i...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 13.1-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-09 13:39 UTC by Yuval Pavel Zholkover
Modified: 2023-09-06 20:56 UTC (History)
5 users (show)

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


Attachments
bazil/fuse workaround (577 bytes, patch)
2022-08-09 13:39 UTC, Yuval Pavel Zholkover
no flags Details | Diff
Standalone test case (559 bytes, text/plain)
2023-07-09 20:23 UTC, Alan Somers
no flags Details
Better test case. It tests both setting and clearing flags. (818 bytes, text/plain)
2023-07-09 20:47 UTC, Alan Somers
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yuval Pavel Zholkover 2022-08-09 13:39:27 UTC
Created attachment 235799 [details]
bazil/fuse workaround

(Copied from https://github.com/golang/go/issues/54100#issuecomment-1200165500)

I ran the bazil/fuse/examples/hellofs example (https://github.com/bazil/fuse/blob/master/examples/hellofs/hello.go), under truss, getting the same result:
# truss -SdHf -o /tmp/truss ./hellofs -fuse.debug /tmp/tst1
2022/07/30 14:30:17 resource temporarily unavailable

 3678 100150: 0.032046504 open("/dev/fuse",O_RDWR|O_CLOEXEC,00) = 3 (0x3)

 3678 100150: 0.032667484 fstat(3,{ mode=crw-rw-rw- ,inode=102,size=0,blksize=4096 }) = 0 (0x0)
 3678 100150: 0.032856138 kqueue()               = 4 (0x4)
 3678 100150: 0.033037163 fcntl(4,F_SETFD,FD_CLOEXEC) = 0 (0x0)


 3678 100150: 0.033541314 compat11.kevent(4,{ 3,EVFILT_READ,EV_ADD|EV_CLEAR,0,0,0x827253f68 3,EVFILT_WRITE,EV_ADD|EV_CLEAR,0,0,0x827253f68 },2,0x0,0,0x0) = 0 (0x0)
 3678 100150: 0.033721862 fcntl(3,F_GETFL,)      = 2 (0x2)
 3678 100150: 0.033866169 fcntl(3,F_SETFL,O_RDWR|O_NONBLOCK) ERR#19 'Operation not supported by device'

The fcntl(3,F_SETFL,O_RDWR|O_NONBLOCK) reports an error, but still sets the underlying file status flag to non-blocking. This later causes a read to return EAGAIN which breaks the program.
Comment 1 Yuval Pavel Zholkover 2022-08-09 14:17:52 UTC
Comment on attachment 235799 [details]
bazil/fuse workaround

With this patch applied, the file descriptor is never attempted to be put into nonblocking mode. And no EAGAIN is observed later which allows the program to mount the fusefs.
Comment 2 Alan Somers freebsd_committer freebsd_triage 2022-08-09 14:23:00 UTC
You attached that patch to the wrong bug.  This bug is about fcntl's behavior with /dev/fuse .  Patches to ports should go in the original rclone bug, or in a new one.
Comment 3 Yuval Pavel Zholkover 2022-08-09 14:27:33 UTC
(In reply to Alan Somers from comment #2)
I've added it here because I think it helps with the reproducer (controlling the open flags directly without the netpoller). It is the same as the one in bug #258056
Comment 4 Alan Somers freebsd_committer freebsd_triage 2022-08-12 21:05:45 UTC
Opening /dev/fuse with O_NONBLOCK works fine.  It just doesn't support switching into non-blocking mode after opening.  I could add that feature, though.  Would that help you?
Comment 5 Yuval Pavel Zholkover 2022-08-12 21:52:58 UTC
(In reply to Alan Somers from comment #4)
Personally I think it is fine to not support changing the device after the initial open. My point about opening the bug is the side-effect of calling F_SETFL with O_NONBLOCK has:

flow 1:
open() blocking
read() - puts the thread to sleep

flow 2:
open() O_NONBLOCK
kqueue+read() when ready - returns EAGAIN, as expeced when no more data is available.

flow 3:
open() blocking
fcntl(F_SETFL, O_NONBLOCK) - returning an error
read() - returns EAGAIN, instead of putting the thread to sleep

flow 3 shouldn't behave differently than flow 1 (other than the fcntl returning an error).
Comment 6 Alan Somers freebsd_committer freebsd_triage 2023-07-09 20:23:02 UTC
Created attachment 243323 [details]
Standalone test case

This is not FUSE-specific.  The bug is in kern_fcntl.  It sets fp->f_flag to the new flags, then calls the file descriptor's ioctl handler.  But if that fails, it doesn't return fp->f_flag to the original value.  The ioctl will fail for any character device that doesn't implement .d_ioctl .  /dev/fuse doesn't, and there are 32 others that don't either.  One example is everything under /dev/led/* (from sys/dev/led/led.c).

Steps to Reproduce
==================

cc -O2 -Wall -g -o f_setfl_o_nonblock f_setfl_o_nonblock.c
./f_setfl_o_nonblock /dev/led/ahci0.0.locate 
F_SETFL failed.  Flags changed from 0 to 0x4.
./f_setfl_o_nonblock /dev/fuse 
F_SETFL failed.  Flags changed from 0 to 0x4.

Since F_SETFL failed, the flags should be unchanged.
Comment 7 Alan Somers freebsd_committer freebsd_triage 2023-07-09 20:47:30 UTC
Created attachment 243325 [details]
Better test case.  It tests both setting and clearing flags.

Attached a better test case.  It tests both setting and clearing flags.
Comment 8 commit-hook freebsd_committer freebsd_triage 2023-07-10 14:15:06 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6c049996ec29bad4a913b019a28f211ab84b0d3d

commit 6c049996ec29bad4a913b019a28f211ab84b0d3d
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2023-07-09 20:48:10 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2023-07-10 14:13:58 +0000

    During F_SETFL, don't change file flags on error

    Previously, even if the FIONBIO or FIOASYNC ioctl failed, the file's
    f_flags variable would still be changed.  Now, kern_fcntl will restore
    the original flags if the ioctl fails.

    PR:             265736
    Reported by:    Yuval Pavel Zholkover <paulzhol@gmail.com>
    MFC after:      2 weeks
    Reviewed by:    kib
    Differential Revision: https://reviews.freebsd.org/D40955

 sys/kern/kern_descrip.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Comment 9 commit-hook freebsd_committer freebsd_triage 2023-09-06 20:47:02 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=14ca2bc402bb82fb59a9aede05f62cb5e634b710

commit 14ca2bc402bb82fb59a9aede05f62cb5e634b710
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2023-07-09 20:48:10 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2023-09-06 20:45:11 +0000

    During F_SETFL, don't change file flags on error

    Previously, even if the FIONBIO or FIOASYNC ioctl failed, the file's
    f_flags variable would still be changed.  Now, kern_fcntl will restore
    the original flags if the ioctl fails.

    PR:             265736
    Reported by:    Yuval Pavel Zholkover <paulzhol@gmail.com>
    Reviewed by:    kib
    Differential Revision: https://reviews.freebsd.org/D40955

    (cherry picked from commit 6c049996ec29bad4a913b019a28f211ab84b0d3d)

 sys/kern/kern_descrip.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Comment 10 Alan Somers freebsd_committer freebsd_triage 2023-09-06 20:56:21 UTC
Not bothering to MFC to stable/12.  Reopen if there's a need.