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

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 <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.
> 
> So will you handle this?

I can.  Do you think this is a good idea? :)

-- 
John Baldwin