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?
