Date: Mon, 22 Sep 2025 09:41:18 +0100 From: John Baldwin <jhb@FreeBSD.org> To: Konstantin Belousov <kib@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 40a42785dbba - main - fcntl(F_SETFL): only allow one thread to perform F_SETFL Message-ID: <92831372-745d-4612-b38f-aeb235dd8cca@FreeBSD.org> In-Reply-To: <202509191419.58JEJsvj031867@gitrepo.freebsd.org> References: <202509191419.58JEJsvj031867@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <kib@FreeBSD.org> > AuthorDate: 2025-09-11 10:05:04 +0000 > Commit: Konstantin Belousov <kib@FreeBSD.org> > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?92831372-745d-4612-b38f-aeb235dd8cca>