On 9/19/25 10:19, Konstantin Belousov wrote:
The branch main has been updated by kib:
URL:
https://cgit.FreeBSD.org/src/commit/?id=40a42785dbba93cc5196178fc49d340c1a89cabe
commit 40a42785dbba93cc5196178fc49d340c1a89cabe
Author: Konstantin Belousov <[email protected]>
AuthorDate: 2025-09-11 10:05:04 +0000
Commit: Konstantin Belousov <[email protected]>
CommitDate: 2025-09-19 14:19:13 +0000
fcntl(F_SETFL): only allow one thread to perform F_SETFL
Use f_vflags file locking for this.
Allowing more than one thread handling F_SETFL might cause de-sync
between real driver state and flags.
Reviewed by: markj
Tested by: pho
Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks
Differential revision: https://reviews.freebsd.org/D52487
Thanks for fixing this. I still slightly worry that "home-grown" locks
aren't visible to WITNESS and it's checking.
I was also expecting this to require more changes, but apparently if a
process directly invokes FIONBIO on a file descriptor, f_flags isn't
updated currently. I wonder if that is a bug. (Similarly for FIOASYNC.)
Oh, we do handle that, but poorly. We don't revert on errors, and this
should be updated to use fsetfl_lock now I think:
kern_ioctl(...)
{
...
switch (com) {
...
case FIONBIO:
if ((tmp = *(int *)data))
atomic_set_int(&fp->f_flag, FNONBLOCK);
else
atomic_clear_int(&fp->f_flag, FNONBLOCK);
data = (void *)&tmp;
break;
case FIOASYNC:
if ((tmp = *(int *)data))
atomic_set_int(&fp->f_flag, FASYNC);
else
atomic_clear_int(&fp->f_flag, FASYNC);
data = (void *)&tmp;
break;
}
error = fo_ioctl(fp, com, data, td->td_ucred, td);
out:
I think instead we want something like:
int f_flag;
switch (com) {
...
case FIONBIO:
case FIOASYNC:
fsetfl_lock(fp);
tmp = *(int *)data;
f_flag = com == FIONBIO ? FNONBLOCK : FASYNC;
if ((fp->f_flag & f_flag) != 0) {
fsetfl_unlock(fp);
goto out;
}
data = (void *)&tmp;
break;
}
error = fo_ioctl(fp, com, data, td->td_ucred, td);
switch (com) {
...
case FIONBIO:
case FIOASYNC:
if (error == 0) {
if (tmp)
atomic_set_int(&fp->f_flag, f_flag);
else
atomic_clear_int(&fp->f_flag, f_flag);
}
fsetfl_unlock(fp);
break;
}
out:
This only updates the flag if the underlying ioctl succeeds, and it also
avoids invoking the underlying ioctl if the flag is already in the correct\
state.
--
John Baldwin
