Re: git: 40a42785dbba - main - fcntl(F_SETFL): only allow one thread to perform F_SETFL

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