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

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 <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) {

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.
> 

-- 
John Baldwin