On 9/22/25 17:05, Konstantin Belousov wrote:
On Mon, Sep 22, 2025 at 09:50:09AM +0100, John Baldwin wrote:
On 9/22/25 04:41, John Baldwin wrote:
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) {
This is wrong, should be:
if (((fp->f_flag & f_flag) != 0) == (tmp != 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)
Probably 'if (tmp != 0)'
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.
So will you handle this?
I can. Do you think this is a good idea? :)
--
John Baldwin
